Skywalker13

Diary of an ex-GeeXboX developer…

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. CelĂ  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 suivrent 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 baclĂ© (et surtout, buggĂ©).

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 apparait. 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 celĂ .

@@ -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, regadons 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 celĂ  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 buggĂ©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 occured 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 occured 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 occured 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 occured 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 occured 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 occured 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 occured 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 occured 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.