Le bon code selon Linus Torvalds
mar, 03/11/2015 - 13:51
Le code du noyau Linux 4.3 vient d'être validé. Une validation précédée par un échange au langage fleuri dont Linus Torvalds a le secret.
Sur LKLM, la liste de diffusion du noyau Linux, Linus écrit ce qu'il pense d'un morceau de code qui devait venir dans cette nouvelle version du noyau. Un tout petit bout de code qui aurait pu tout à fait passer inaperçu. Seulement voilà, Linus l'a vu, et il ne pouvait pas le laisser passer, dit-il.
Ce code vient apporter une correction à net/ipv6/ip6_output.c:
mtu -= hlen + sizeof(struct frag_hdr);
Voici le nouveau code incriminé :
if (overflow_usub(mtu, hlen + sizeof(struct frag_hdr), &mtu) ||
mtu <= 7)
goto fail_toobig;
C'est juste de la m* et il génère un code de m*, il est laid, et il n'y a aucune raison pour qu'il soit là, dit Linus.
Comme Linus a en plus rencontré un problème de compilation sur ce code, il ajoute que ce code ne devrait pas utiliser des fantaisies qui nécessitent un compilateur intégrant le support de fonctionnalités magiques.
Pour lui tous ceux qui pensent que le code ci-dessus est :
(a) lisible
(b) efficace (même avec le compilateur magique)
(c) particulièrement sûr
sont incompétents et à côté de la plaque, écrit-il.
Pour Linus, le code ci-dessus n'a pour seul raison d'être que d'utiliser overflow_usub et c'est une très mauvaise excuse : it's a f*cking bad excuse for that braindamage.
Voilà pour l'essentiel du pittoresque message de Linus, qu'il est intéressant de lire en entier.
Bien sûr, il y a la manière Linus Torvalds, contestable et souvent contestée. Cela a même donné lieu à un fork du noyau récemment. Il y a également le langage qui pourrait être un peu plus châtié.
Mais il y a aussi que Linux est un super noyau, et que cela ne s'obtient pas sans écrire du bon code. Linus propose le sien. 3 lignes en remplacement des 3 lignes de m* :-)
if (mtu < hlen + sizeof(struct frag_hdr) + 8)
goto fail_toobig;
mtu -= hlen + sizeof(struct frag_hdr);
Il faut bien reconnaître, de l'humble avis de votre serviteur, que ce code est beaucoup plus lisible, maintenable et sain.
N'est-ce pas ?
Commentaires
Dans les 2 cas on tape dans le "magic number" qui est un anti-pattern reconnu, donc bof
Certes, mais dans les 2 cas, le "magic number" n'est pas le sujet de la discussion.
J'aurais toujours du mal a considérer comme un bon code ceux comportant des GOTO et des chiffres en clair.
Le premier code me semble cependant meilleur que celui de torval, car il est plus général.
La présence de GOTOs qui pointent vers un et un seul label pour le traitement d'erreurs et seulement dans ce cas, au sein d'une fonction C complexe est une pratique admise et correcte.
Bjarne Stroustrup, qui aime bien donner des conseils de programmation en C++ l'explique dans son livre The C++ Language, pour justifier pourquoi il a gardé le goto de C dans C++ (outre la question de la compatibilité avec le C, bien sûr)
Le chiffre en clair ne devrait pas être là, c'est sûr, ainsi que d'autres l'ont relevé dans leurs commentaires. Mais ce n'est pas le sujet.
Sans vouloir me faire l'avocat du diable, car à mon humble avis Linus Torvalds devrait corriger ses mauvaises pratiques du langage humain :-), ce qu'il veut surtout dire dans son post sur LKLM c'est que le code incriminé est difficile à relire, à comprendre et donc à maintenir, avec risque d'introduction de bugs. Sur ce point il me semble qu'il a raison. Dans le cas présent, que le code soit plus général ne me semble pas un avantage.
Les goto dans certain cas peuvent être utiles je trouve, c est peut être pas une bonne pratique mais je les utilisent pour limiter les return quand il y en a plus plusieurs avec un goto vers la fin des fonctions ou pour close un fichier dans des fonctions avec de nombreux test pour ne pas oublier une action avant la sortie de la fonction et ne pas avoir besoin d écrire une fonction spécifique
Après effectivement ça oblige a fairee des alloc de variables pour le retour mais plus facile a maintenir et le coût reste faible je trouve évidemment c est juste mon avis de petit dev ça évoluera peut être dans le temps ;)
Tellement plus clair, et surtout pas besoin de connaître ce que fait overflow_usub fonction que je n avais encore jamais vu perso donc clairement plus maintenable car compréhensible par casiment tous les dev
Donc il le dit a ça manière mais il a tellement raison