Travailler seul sur du code source fermé ~ 🇫🇷

Posted at — Sep 1, 2023

Il y a longtemps, quand j’étais apprenti (en 2002), mon patron de stage me tenait tête à propos de Linux et du libre en général. Pour lui Linux avait été créé par une bande d’étudiants et donc que ce système ne pouvait pas être digne d’intérêt. Il était déjà très clair pour moi qu’il parlait en toute ignorance. Et depuis plus de 20 ans, je suis totalement convaincu que le code source ouvert et libre de projets conséquents est toujours de bien meilleur qualité que ne le sera jamais aucun code source fermé. Dans cet article je vais justement en parler pour un cas de figure malheureusement typique, auquel j’ai été confronté il y a quelques semaines.

Au début, il y avait une seule personne

J’ai du réaliser des adaptations dans un projet (une migration). Le code source que je vais vous montrer ici a été écrit il y a plusieurs années et est utilisé (aujourd’hui même) en production.

Je me permet de le présenter ici car, quand je suis tombé dessus, ce code m’a étouffé (et pourtant il est très court). Je veux en parler car je trouve qu’il représente parfaitement ce qu’on ne devrait pas faire et que c’est alors un excellent cas d’école. Cela montre aussi l’importance d’avoir toujours au moins une personne pour jeter à oeil à ce que l’on fait. Le “code review” est indispensable et justement, dans le monde du libre tel que Linux, c’est une étape obligatoire contrairement à des projets fermés où les développeurs peuvent y faire à peu prêt tout et surtout n’importe quoi.

La fonction

Voici ce qui me fait mal aux yeux. Peut être que ce n’est pas le cas pour vous et j’en suis navré. Mais si vous regardez mieux cette fonction vous pourriez y trouver assez vite des problèmes.

Je n’ai même pas besoin de chercher plus loin que la première ligne. Je vais aborder pas à pas ce que je pense être des problèmes et comment je vais les résoudre.

Ce qu’il faut savoir avant de commencer, c’est que cette fonction C doit retourner 1 si le processus est en cours d’exécution, sinon elle doit retourner 0. Elle peut aussi retourner d’autres valeurs en cas d’erreurs.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (processId != 0) {
    auto res = kill(processId, 0);

    if (res == 0) {
      char procNameByPidBuffer[1024];
      int ret = get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

      if (ret == 0) {
        // No process with the pid has been found
        return 0;
      } else if (ret == 1) {
        // Process with pid has been found, but what about its name?
        if (strstr(procNameByPidBuffer, processName) == NULL) {
          // Process with pid found, but different name
          return 0;
        }
      } else {
        // An error occurred
        return ret;
      }
    } else {
      int errsv = errno;
      if (errsv == ESRCH) {
        // Process with pid is not running
        return 0;
      } else {
        return errsv;
      }
    }
  }

  return 1;
}

Qu’est-ce que je vois ?

Je me dis tout de suite, “Oh non, pas encore… ?!”. Ici il y a des imbrications de if qui semblent suivre la pensée du développeur qui a écrit le code. Ce que je veux dire c’est que (pour moi en tout cas) il y a deux grandes phases très rapprochées à l’écriture de n’importe quel code source. D’abord il y a le cheminement que l’on a dans la tête et que l’on souhaite retranscrire en code, ensuite il y a la réorganisation du code pour le rendre clair.

Dans le code présenté ci-dessus, j’imagine que le développeur l’a écrit comme il l’a pensé et à sauté l’étape de nettoyage. C’est typique de code écrit rapidement (dans le rush). Tout juste testé pour les quelques cas nécessaires à ce moment là. Bref, c’est juste du code bâclé (et surtout, bugué).

Bien entendu, il est normal d’implémenter comme on pense, mais rapidement il faut revenir sur le code pour le rendre lisible et efficace. C’est pendant cette phase (négligée ici) qu’on découvre les bugs que la pensée du moment ne permettait pas d’imaginer. Je vous propose alors de faire ce travail avec moi, étape par étape.

Guard Clause Pattern

Vous vous rappelez de Gandalf face au Balrog ? Et bien c’est le Guard Clause Pattern. C’est certainement mon pattern préféré et malheureusement pas assez mis en pratique. Je vous donne une définition par ChatGPT qui résume très bien ce que c’est.

Le “Guard Clause Pattern”, également connu en français sous le nom de “Motif de Clause de Garde”, est un concept de programmation qui consiste à utiliser des conditions préliminaires pour vérifier rapidement et traiter les cas d’erreurs ou de conditions inattendues avant d’entrer dans le corps principal d’une fonction ou d’une méthode. Il vise à améliorer la lisibilité du code en évitant des niveaux excessifs d’indentation et en traitant les cas exceptionnels en premier. En résumé, il s’agit d’une technique de gestion des erreurs et des conditions spéciales au début d’une fonction pour rendre le code plus clair et plus facile à comprendre.

– ChatGPT-3.5

J’ai appris cette technique dans les années 2006-2009 quand je travaillais au sein du projet GeeXboX. Un des développeurs l’appliquait dans tout ce qu’il faisait. Ca m’a séduit et j’essaie aussi de l’appliquer autant que possible depuis lors, et j’espère que j’arriverai ici à convaincre les plus sceptiques.

Pour en revenir à la fonction initiale et à cette fameuse “première ligne” avec ce if, c’est d’effectuer ledit exercice.

A chaque étape, je donne des explications ainsi que le diff associé aux modifications, puis la fonction complète.

1. Inversions de conditions

On inverse les conditions afin de sortir au plus vite en cas d’erreur. processId != 0 devient !processId (certains préfère processId == 0, c’est comme vous préférez).

@@ -6,3 +6,5 @@
 int is_native_process_running(int processId, LPCSTR processName) {
-  if (processId != 0) {
+  if (!processId)
+    return 1;
+
     auto res = kill(processId, 0);
@@ -35,3 +37,2 @@
       }
-    }
   }

Avec cette opération, le return 1 tout à la fin est un peu confus. On le laisse pour le moment car on y reviendra plus tard.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  auto res = kill(processId, 0);

  if (res == 0) {
    char procNameByPidBuffer[1024];
    int ret =
        get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

    if (ret == 0) {
      // No process with the pid has been found
      return 0;
    } else if (ret == 1) {
      // Process with pid has been found, but what about its name?
      if (strstr(procNameByPidBuffer, processName) == NULL) {
        // Process with pid found, but different name
        return 0;
      }
    } else {
      // An error occurred
      return ret;
    }
  } else {
    int errsv = errno;
    if (errsv == ESRCH) {
      // Process with pid is not running
      return 0;
    } else {
      return errsv;
    }
  }

  return 1;
}

Juste en faisant cette inversion, quelque chose de bizarre apparaît. Quand processId vaut 0, la fonction retourne 1. Cette fonction considère que le processus 0 est en cours d’exécution. Uh ?! Le processus 0 ??? On a notre premier bug, mais continuons avec if (res == 0).

2. Inversions de conditions et simplifications

On inverse aussi cette condition et on voit clairement qu’en erreur, on va forcément sur un return; simplifions tout cela.

@@ -11,3 +11,9 @@

-  if (res == 0) {
+  if (res) {
+    int errsv = errno;
+    if (errsv == ESRCH)
+      return 0; // Process with pid is not running
+    return errsv;
+  }
+
     char procNameByPidBuffer[1024];
@@ -29,11 +35,2 @@
     }
-  } else {
-    int errsv = errno;
-    if (errsv == ESRCH) {
-      // Process with pid is not running
-      return 0;
-    } else {
-      return errsv;
-    }
-  }

Avec cette opération on visualise bien le Guard Clause Pattern. La gestion des erreurs se déplace naturellement vers le haut de la fonction.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  auto res = kill(processId, 0);

  if (res) {
    int errsv = errno;
    if (errsv == ESRCH)
      return 0; // Process with pid is not running
    return errsv;
  }

  char procNameByPidBuffer[1024];
  int ret =
      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

  if (ret == 0) {
    // No process with the pid has been found
    return 0;
  } else if (ret == 1) {
    // Process with pid has been found, but what about its name?
    if (strstr(procNameByPidBuffer, processName) == NULL) {
      // Process with pid found, but different name
      return 0;
    }
  } else {
    // An error occurred
    return ret;
  }

  return 1;
}

3. Variable inutile et l’opérateur ternaire

Il y a une variable inutile (errsv) et on peut également utiliser un opérateur ternaire.

@@ -9,10 +9,6 @@

+  // If errno == ESRCH, process with pid is not running
   auto res = kill(processId, 0);
-
-  if (res) {
-    int errsv = errno;
-    if (errsv == ESRCH)
-      return 0; // Process with pid is not running
-    return errsv;
-  }
+  if (res)
+    return errno == ESRCH ? 0 : errno;

Le code devient bien plus conscis.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  // If errno == ESRCH, process with pid is not running
  auto res = kill(processId, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  char procNameByPidBuffer[1024];
  int ret =
      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

  if (ret == 0) {
    // No process with the pid has been found
    return 0;
  } else if (ret == 1) {
    // Process with pid has been found, but what about its name?
    if (strstr(procNameByPidBuffer, processName) == NULL) {
      // Process with pid found, but different name
      return 0;
    }
  } else {
    // An error occurred
    return ret;
  }

  return 1;
}

Bien, regardons ce bout de code. Il est un peu bizarre, vous ne trouvez pas ? Si errno est différent de ESRCH alors on le retourne. Mais cette fonction, d’après l’auteur, est censée retourner 1 si le processus est en cours d’exécution. Est-ce qu’il faut comprendre que errno ne peut jamais prendre la valeur 1 ?

Et bien c’est un bug, quand errno vaut 1 cela signifie que l’erreur EPERM s’est produite. Cette erreur est générée par exemple, si vous utilisez cette fonction avec un processus d’un utilisateur où vous n’avez pas les droits. Par hasard ici, ca fonctionne presque. Imaginez que vous voulez savoir si le PID 1 est en cours d’exécution. Vous n’avez pas les droits, et donc EPERM est généré (avec la valeur 1). La fonction dit que ce processus est en cours d’exécution.

Alors oui, c’est bien le cas sinon on n’aurait pas reçu le code 1. Donc oui, le PID 1 est bien en cours d’exécution, mais…

On arrive au bug suivant. L’argument processName n’a pas été comparé. En effet, cette fonction utilise processName pour déterminer si un processus d’un certain PID, contient le nom passé en paramètre pour dire si oui ou non ce processus est en cours d’exécution. La raison est simple. Quand un processus se termine, il est tout à fait possible qu’un autre processus prenne sa place avec un PID identique. Se fier uniquement au PID n’est pas suffisant.

En conclusion pour ce bug on peut dire que cette fonction retourne 1 avec n’importe quel nom de processus dès le moment qu’on n’a pas les droits pour lui envoyer des signaux POSIX.

Je vous propose qu’on garde ce bug au chaud et qu’on continue le nettoyage. On y reviendra plus loin dans cet article.

4. Un else avec un return; inutile et confus

On prend le if suivant qui nous montre que ret == 0 est une sortie. Il y a un return immédiatement après. On enlève ce else inutile qui altère la lecture.

@@ -18,6 +18,6 @@

-  if (ret == 0) {
-    // No process with the pid has been found
-    return 0;
-  } else if (ret == 1) {
+  if (!ret)
+    return 0; // No process with the pid has been found
+
+  if (ret == 1) {
     // Process with pid has been found, but what about its name?

Ici on n’a vraiment rien changé.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  // If errno == ESRCH, process with pid is not running
  auto res = kill(processId, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  char procNameByPidBuffer[1024];
  int ret =
      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

  if (!ret)
    return 0; // No process with the pid has been found

  if (ret == 1) {
    // Process with pid has been found, but what about its name?
    if (strstr(procNameByPidBuffer, processName) == NULL) {
      // Process with pid found, but different name
      return 0;
    }
  } else {
    // An error occurred
    return ret;
  }

  return 1;
}

Pour la suite, on part du principe que la fonction get_native_process_name_by_pid n’est pas buggée.

5. Double if avec un else invisible car noyé par l’accolade

Le if (ret == 1) est bien un cas de succès. Il y a par contre un double if pas très heureux car il y a un chemin (avec le second if, quand la condition est false) qui demande de scanner avec l’oeil jusqu’en bas de la fonction. Je vous invite ici à revoir le code original (tout au sommet de cet article), où se scan avec l’oeil est bien plus pénible. Même si ce code est correcte, la logique est cachée dans la cascade. Alors on va simplifier…

@@ -21,9 +21,7 @@

-  if (ret == 1) {
     // Process with pid has been found, but what about its name?
-    if (strstr(procNameByPidBuffer, processName) == NULL) {
       // Process with pid found, but different name
+  if (ret == 1 && !strstr(procNameByPidBuffer, processName))
       return 0;
-    }
-  } else {
+
     // An error occurred
@@ -31,4 +29 @@
   }
-
-  return 1;
-}

Avec ce changement, on profite de supprimer le dernier return qui ne fait plus de sens.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  // If errno == ESRCH, process with pid is not running
  auto res = kill(processId, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  char procNameByPidBuffer[1024];
  int ret =
      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

  if (!ret)
    return 0; // No process with the pid has been found

  // Process with pid has been found, but what about its name?
  // Process with pid found, but different name
  if (ret == 1 && !strstr(procNameByPidBuffer, processName))
    return 0;

  // An error occurred
  return ret;
}

Bien, stoppons nous ici pour réfléchir. La fonction qui rend ret est en réalité aussi buguée. Admettons qu’elle soit corrigée. Elle rend 0 ou 1 quand tout va bien, le reste sont des erreurs. Alors traitons l’erreur avant tout.

6. Garder le meilleur pour la fin

On peut très simplement fusionner le cas du retour (de ret) à 0 ou != 1 car seul le cas 1 indique qu’il faut continuer à chercher si le processus est bien en cours d’exécution. Le test suivant ret == 1 devient inutile.

@@ -18,4 +18,6 @@

-  if (!ret)
-    return 0; // No process with the pid has been found
+  // No process with the pid has been found (ret == 0), or an error occurred if
+  // it's not 1
+  if (ret != 1)
+    return ret;

@@ -23,7 +25,6 @@
   // Process with pid found, but different name
-  if (ret == 1 && !strstr(procNameByPidBuffer, processName))
+  if (!strstr(procNameByPidBuffer, processName))
     return 0;

-  // An error occurred
-  return ret;
+  return 1;
 }

Ainsi la variable ret n’est plus propagée jusqu’à la fin de la fonction.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  // If errno == ESRCH, process with pid is not running
  auto res = kill(processId, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  char procNameByPidBuffer[1024];
  int ret =
      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

  // No process with the pid has been found (ret == 0), or an error occurred if
  // it's not 1
  if (ret != 1)
    return ret;

  // Process with pid has been found, but what about its name?
  // Process with pid found, but different name
  if (!strstr(procNameByPidBuffer, processName))
    return 0;

  return 1;
}

7. Le ternaire, éternel allié

On va faire encore de la poutze. Utilisons un ternaire… Il suffit dans ce cas de vérifier le nom du processus. Si le nom est trouvé on retourne 1, sinon on retourne 0.

@@ -24,7 +24,3 @@
   // Process with pid has been found, but what about its name?
-  // Process with pid found, but different name
-  if (!strstr(procNameByPidBuffer, processName))
-    return 0;
-
-  return 1;
+  return strstr(procNameByPidBuffer, processName) ? 1 : 0;
 }

N’est-ce pas plus élégant ? Certains iraient même plus loin et écriraient :

return !!strstr(procNameByPidBuffer, processName);

Je vous épargne cette écriture pour cette fois. Néanmoins c’est une façon tout à fait reconnue par les experts en C pour produire un booléen.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int processId, LPCSTR processName) {
  if (!processId)
    return 1;

  // If errno == ESRCH, process with pid is not running
  auto res = kill(processId, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  char procNameByPidBuffer[1024];
  int ret =
      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);

  // No process with the pid has been found (ret == 0), or an error occurred if
  // it's not 1
  if (ret != 1)
    return ret;

  // Process with pid has been found, but what about its name?
  return strstr(procNameByPidBuffer, processName) ? 1 : 0;
}

J’espère que vous êtes d’accord avec moi, que cette fonction commence à ressembler à quelque chose. On peut encore faire plus de poutze.

8. Attention, auto en C ce n’est pas auto de C++

Les variables ret et res sont des int alors simplifions. On déplace les déclarations au sommet, on renomme l’évidence (le mot process dans le nom des variables est redondant avec le nom de la fonction) et on calcul la taille du tableau avec sizeof.

@@ -5,4 +5,7 @@
  */
-int is_native_process_running(int processId, LPCSTR processName) {
-  if (!processId)
+int is_native_process_running(int pid, LPCSTR name) {
+  int res;
+  char procName[1024];
+
+  if (!pid)
     return 1;
@@ -10,3 +13,3 @@
   // If errno == ESRCH, process with pid is not running
-  auto res = kill(processId, 0);
+  res = kill(pid, 0);
   if (res)
@@ -14,13 +17,10 @@

-  char procNameByPidBuffer[1024];
-  int ret =
-      get_native_process_name_by_pid(processId, procNameByPidBuffer, 1024);
-
   // No process with the pid has been found (ret == 0), or an error occurred if
   // it's not 1
-  if (ret != 1)
-    return ret;
+  res = get_native_process_name_by_pid(pid, procName, sizeof(procName));
+  if (res != 1)
+    return res;

   // Process with pid has been found, but what about its name?
-  return strstr(procNameByPidBuffer, processName) ? 1 : 0;
+  return strstr(procName, name) ? 1 : 0;
 }

J’ai également enlevé le mot clef auto. En effet, ici c’est de l’ignorance du développeur qui a écrit ce code. Le modificateur auto en C n’a absolument pas la même signification que auto en C++. En C, auto indique que la variable déclarée doit être local (c’est implicite). En C++, auto sert à faire de la déduction automatique du type.

Si le code ci-dessus fonctionne c’est uniquement parce qu’en C, ne pas spécifier de type est équivalent à int et par hasard c’est bien ce que l’on souhaite.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int pid, LPCSTR name) {
  int res;
  char procName[1024];

  if (!pid)
    return 1;

  // If errno == ESRCH, process with pid is not running
  res = kill(pid, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  // No process with the pid has been found (ret == 0), or an error occurred if
  // it's not 1
  res = get_native_process_name_by_pid(pid, procName, sizeof(procName));
  if (res != 1)
    return res;

  // Process with pid has been found, but what about its name?
  return strstr(procName, name) ? 1 : 0;
}

Vous vous rappelez ? Je vous avais dis de garder un bug au chaud. Le bug qui fait en sorte que la fonction retourne toujours 1 si errno vaut EPERM. Mais avant de le régler, on va s’occuper du bug au point 1.

9. Le PID 0 n’existe pas

S’il n’y a pas de PID, il faut retourner 0 et certainement pas 1. Le PID 0 ne peut pas exister, c’est une certitude.

@@ -10,3 +10,3 @@
   if (!pid)
-    return 1;
+    return 0;

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int pid, LPCSTR name) {
  int res;
  char procName[1024];

  if (!pid)
    return 0;

  // If errno == ESRCH, process with pid is not running
  res = kill(pid, 0);
  if (res)
    return errno == ESRCH ? 0 : errno;

  // No process with the pid has been found (ret == 0), or an error occurred if
  // it's not 1
  res = get_native_process_name_by_pid(pid, procName, sizeof(procName));
  if (res != 1)
    return res;

  // Process with pid has been found, but what about its name?
  return strstr(procName, name) ? 1 : 0;
}

Que faire avec EPERM ? Voici une proposition qui ne casse pas l’ABI.

10. Errno ou moins errno ?

Quand errno n’est pas ESRCH, alors on retourne -errno ainsi on a plus du tout de confusion entre la valeur 1 du succès avec le code d’erreur EPERM. Mais avant de tester ESRCH, on s’assure que errno ne vaut pas EPERM. L’idée est simple, si on reçoit EPERM alors le processus existe, il faut continuer pour vérifier son nom.

@@ -13,5 +13,7 @@
   // If errno == ESRCH, process with pid is not running
+  // if errno is EPERM == 1: Operation not permitted, we know that
+  //   the process exists, then we continue
   res = kill(pid, 0);
-  if (res)
-    return errno == ESRCH ? 0 : errno;
+  if (res && errno != EPERM)
+    return errno == ESRCH ? 0 : -errno;

Désormais on est sûr de ne plus jamais retourner 1 en cas d’erreur avec la fonction kill.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int pid, LPCSTR name) {
  int res;
  char procName[1024];

  if (!pid)
    return 0;

  // If errno == ESRCH, process with pid is not running
  // if errno is EPERM == 1: Operation not permitted, we know that
  //   the process exists, then we continue
  res = kill(pid, 0);
  if (res && errno != EPERM)
    return errno == ESRCH ? 0 : -errno;

  // No process with the pid has been found (ret == 0), or an error occurred if
  // it's not 1
  res = get_native_process_name_by_pid(pid, procName, sizeof(procName));
  if (res != 1)
    return res;

  // Process with pid has been found, but what about its name?
  return strstr(procName, name) ? 1 : 0;
}

On pourrait s’arrêter là mais j’aime enlever le bruit.

11. Moins de bruit s’il vous plaît

Initialisons les variables avec une chaîne vide et res à 0.

@@ -6,4 +6,4 @@
 int is_native_process_running(int pid, LPCSTR name) {
-  int res;
-  char procName[1024];
+  int res = 0;
+  char procName[1024] = {0};

Ici vous voyez l’astuce pour initialiser un tableau avec la valeur 0. On évite ainsi d’avoir du bruit qui pourrait ressembler à une chaîne de caractères.

/***********************************************************************
 *           is_native_process_running
 *
 * Return if a native process is currently running, given its pid and name.
 */
int is_native_process_running(int pid, LPCSTR name) {
  int res = 0;
  char procName[1024] = {0};

  if (!pid)
    return 0;

  // If errno == ESRCH, process with pid is not running
  // if errno is EPERM == 1: Operation not permitted, we know that
  //   the process exists, then we continue
  res = kill(pid, 0);
  if (res && errno != EPERM)
    return errno == ESRCH ? 0 : -errno;

  // No process with the pid has been found (ret == 0), or an error occurred if
  // it's not 1
  res = get_native_process_name_by_pid(pid, procName, sizeof(procName));
  if (res != 1)
    return res;

  // Process with pid has been found, but what about its name?
  return strstr(procName, name) ? 1 : 0;
}

Pour conclure

Je vous invite à reprendre sous les yeux la fonction originale et de la comparer avec la dernière version. La version corrigée respecte le Guard Clause Pattern et est beaucoup plus lisible. Les retours intermédiaires servent à sortir pour dire que le processus n’est pas en fonctionnement ou qu’il y a une erreur non gérée. Le vrai cas de succès est fait uniquement dans le tout dernier retour de la fonction.

Le second point notable vient de la gestion des erreurs de type errno. En faisant -errno on évite de confondre EPERM avec le succès de la fonction (quand le processus existe et que le nom correspond).

D’un point de vue cosmétique, on a éliminé beaucoup de niveaux d’indentations qui n’apportaient que de la confusion, et celà parce qu’on a suivit le Guard Clause Pattern.

Quoi qu’il en soit, nous avons amélioré la fonction tout en évitant de casser l’ABI. Il faudrait néanmoins prévenir les utilisateurs de cette fonction, que les erreurs sont désormais retournées uniquement en tant qu’entier négatifs et qu’ils correspondent à -errno.

Pour être encore plus propre, il faudrait séparer l’information bool (0..1) des codes d’erreur. Une de ces deux information devrait être rendu par référence afin de ne pas mélanger le tout dans un seul et même int.

Skywalker13

Diary of an ex-GeeXboX developer…