Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata

[Robin Roevens <robin.roevens@disroot.org>]

[-- Attachment #1: Type: text/plain, Size: 30342 bytes --]

Hi

Michael Tremer schreef op ma 10-10-2022 om 11:04 [+0100]:
> Hello,
> 
> > On 9 Oct 2022, at 00:09, Robin Roevens <robin.roevens(a)disroot.org>
> > wrote:
> > 
> > Hi Michael
> > 
> > Thanks again for the review and comments.
> > 
> > Michael Tremer schreef op vr 07-10-2022 om 16:01 [+0100]:
> > > Hello again,
> > > 
> > > > On 6 Oct 2022, at 18:59, Robin Roevens
> > > > <robin.roevens(a)disroot.org>
> > > > wrote:
> > > > 
> > > > * Addonctrl will now check in addon metadata for the exact
> > > > initscript
> > > > names (Services). If more than one initscript is defined for an
> > > > addon,
> > > > the requested action will be performed on all listed
> > > > initscripts.
> > > > * Added posibility to perform action on a specific initscript
> > > > of an
> > > > addon instead of on all initscripts of the addon.
> > > > * New action 'list-services' to display a list of services
> > > > related
> > > > to
> > > > an addon.
> > > > * New action 'boot-status' to display wether service(s) are
> > > > enabled
> > > > to start on boot or not.
> > > > * More error checking and cleaner error reporting to user
> > > > * General cleanup and code restructuring to avoid code
> > > > duplication
> > > > * Updated and made usage instructions more verbose.
> > > > 
> > > > Fixes: Bug#12935
> > > > Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> > > > ---
> > > > src/misc-progs/addonctrl.c | 397
> > > > +++++++++++++++++++++++++++++++---
> > > > ---
> > > > 1 file changed, 336 insertions(+), 61 deletions(-)
> > > > 
> > > > diff --git a/src/misc-progs/addonctrl.c b/src/misc-
> > > > progs/addonctrl.c
> > > > index 14b4b1325..1687aac19 100644
> > > > --- a/src/misc-progs/addonctrl.c
> > > > +++ b/src/misc-progs/addonctrl.c
> > > > @@ -10,71 +10,346 @@
> > > > #include <string.h>
> > > > #include <unistd.h>
> > > > #include <sys/types.h>
> > > > +#include <sys/stat.h>
> > > > #include <fcntl.h>
> > > > +#include <dirent.h>
> > > > +#include <fnmatch.h>
> > > > +#include <errno.h>
> > > > #include "setuid.h"
> > > > 
> > > > #define BUFFER_SIZE 1024
> > > > 
> > > > +const char *enabled_path = "/etc/rc.d/rc3.d";
> > > > +const char *disabled_path = "/etc/rc.d/rc3.d/off";
> > > > +
> > > > +char errormsg[BUFFER_SIZE] = "";
> > > > +const char *usage = 
> > > > +  "Usage\n"
> > > > +  "  addonctrl <addon>
> > > > (start|stop|restart|reload|enable|disable|status|boot-
> > > > status|list-
> > > > services) [<service>]\n"
> > > > +  "\n"
> > > > +  "Options:\n"
> > > > +  "  <addon>\t\tName of the addon to control\n"
> > > > +  "  <service>\t\tSpecific service of the addon to control
> > > > (optional)\n"
> > > > +  "  \t\t\tBy default the requested action is performed on all
> > > > related services. See also 'list-services'.\n"
> > > > +  "  start\t\t\tStart service(s) of the addon\n"
> > > > +  "  stop\t\t\tStop service(s) of the addon\n"
> > > > +  "  restart\t\tRestart service(s) of the addon\n"
> > > > +  "  enable\t\tEnable service(s) of the addon to start at
> > > > boot\n"
> > > > +  "  disable\t\tDisable service(s) of the addon to start at
> > > > boot\n"
> > > > +  "  status\t\tDisplay current state of addon service(s)\n"
> > > > +  "  boot-status\t\tDisplay wether service(s) is enabled on
> > > > boot
> > > > or not\n"
> > > > +  "  list-services\t\tDisplay a list of services related to
> > > > the
> > > > addon";
> > > > +
> > > > +// Find a file <path> using <filepattern> as glob pattern. 
> > > > +// Returns the found filename or NULL if not found
> > > > +char *find_file_in_dir(const char *path, const char
> > > > *filepattern) 
> > > > +{
> > > > +  struct dirent *entry;
> > > > +  DIR *dp;
> > > > +  char *found = NULL;
> > > > +
> > > > +  if ((dp = opendir(path)) != NULL) {
> > > > +  while(found == NULL && (entry = readdir(dp)) != NULL)
> > > > +  if (fnmatch(filepattern, entry->d_name, FNM_PATHNAME)
> > > > == 0)
> > > > +  found = strdup(entry->d_name);
> > > > +
> > > > +  closedir(dp);
> > > > +  }
> > > > +
> > > > +  return found;
> > > > +}
> > > > +
> > > > +// Reads Services metadata for <addon>.
> > > > +// Returns pointer to array of strings containing the services
> > > > for
> > > > <addon> 
> > > > +// and sets <servicescnt> to the number of found services
> > > > +char **get_addon_services(const char *addon, int *servicescnt,
> > > > const char *filter) {
> > > > +  const char *metafile_prefix =
> > > > "/opt/pakfire/db/installed/meta-
> > > > ";
> > > > +  const char *metadata_key = "Services";
> > > > +  const char *keyvalue_delim = ":";
> > > > +  const char *service_delim = " ";
> > > > +  char *token;
> > > > +  char **services = NULL;
> > > > +  char *service;
> > > > +  char *line = NULL;
> > > > +  size_t line_len = 0;
> > > > +  int i = 0;
> > > > +  char *metafile;
> > > > + 
> > > > +  if (addon == NULL) {
> > > > +  errno = EINVAL;
> > > > +  return NULL;
> > > > +  }
> > > > +
> > > > +  if (asprintf(&metafile, "%s%s", metafile_prefix, addon) == -
> > > > 1)
> > > > {
> > > > +  errno = ENOMEM;
> > > > +  return NULL;
> > > > +  }
> > > 
> > > You should initialise metafile with NULL before passing it to
> > > asprintf().
> > 
> > I'm not sure why? I can't find any information on why it should be
> > initialized to NULL as it is passed to malloc, which doesn't seem
> > to
> > require it to be NULL beforehand. Or at least I didn't find any
> > documentation stating as such.
> > 
> > What I did find is that many implementations of C leave the string
> > in
> > undefined state when it was unable to malloc. While others (like
> > GNU)
> > set it to NULL explicitly if malloc fails.
> 
> I suppose my concern is rooted in the different implementations of
> such functions.
> 
> Since this program is exclusively running on IPFire and using glibc,
> you might get away with this. However, I try to write my code as
> portable as possible and avoid any uninitialised pointers wherever
> possible (because they are normally a recipe for disaster), that I
> initialise everything.
> 
> Should this be absolutely unnecessary, the compiler will skip the
> initialisation anyways.
> 
> > But since I check the return value of asprintf, I never read or
> > touch
> > the returned string, undefined or not, so I didn't think it was
> > required.
> > 
> > But I'll initialize them. It's probably indeed the safest to do
> > anyway.
> 
> Yes, better be safe than sorry.
> 
> reallocarray() would require this, and so it would be more uniform to
> initialise everything.
> 
> > 
> > > 
> > > asprintf() will automatically set errno, actually.
> > 
> > Ok. I was not sure as the manpage for asprintf does not mention
> > that
> > while the manpage for for example strdup does mention that
> > explicitly.
> 
> Depending on implementation, asprintf might never set it itself, but
> the function it calls (i.e. malloc, etc.).
> 
> > And I have investigated the source of asprintf, and it does not set
> > errno when there is a problem with malloc.. 
> > But now I found that malloc itself actually sets errno. So it will
> > indeed be set by asprintf too as that calls malloc.
> > So I'll remove all my setting of ENOMEM.
> > 
> > > 
> > > > +
> > > > +  FILE *fp = fopen(metafile,"r");
> > > > +  if (fp != NULL) {
> > > 
> > > Just FYI, I like writing this check as “if (!fp)” because it is a
> > > a
> > > lot shorter...
> > 
> > It seems there are 2 camps about this, one just writes what you
> > like to
> > write, while the other argues it is more clear that you are working
> > with pointers by explicitly checking against NULL.
> 
> Yes, there are indeed two camps. One of them is being so funny that
> they set NULL to a value like “5” and check if your code still works.
> 
> My code won’t. But I am sure their code won’t work any more if I
> define 5 as 6, and the number 2 because 9 and free becomes malloc.
> Changing arbitrary things doesn’t make any sense.
> 
> > But if you think that is unnecessary I'll change it.
> 
> This is more advisory than a demand. When reading code, the brain
> optimises for certain patterns and if you recognise those patterns
> again somewhere else, the brain will understand things quicker. So I
> kind of like it when people code in the same style as I do because it
> helps me to review things faster and better.
> 
> > Should I change all my other checks against NULL also? Or only the
> > filepointer ? 
> 
> If you don’t mind doing that, I would like it :)

No problem. I think you are in the right position to demand a specific
code style for use on IPFire  :-)

> 
> > 
> > > 
> > > > +  // Get initscript(s) for addon from meta-file
> > > > +  while (getline(&line, &line_len, fp) != -1 && services ==
> > > > NULL) {
> > > > +  // Strip newline
> > > > +  char *newline = strchr(line, '\n');
> > > > +  if (newline) *newline = 0;
> > > 
> > > Yes, this will cut off the trailing newline.
> > > 
> > > > +
> > > > +  // Parse key/value and look for required key.
> > > > +  token = strtok(line, keyvalue_delim);
> > > > +  if (token != NULL && strcmp(token, metadata_key) == 0)
> > > > {
> > > > +  token = strtok(NULL, keyvalue_delim);
> > > > +  if (token != NULL) {
> > > > +  // Put each service in services array
> > > > +  service = strtok(token, service_delim);
> > > > +  while (service != NULL) {
> > > > +  // if filter is set, only select filtered
> > > > service
> > > > +  if ((filter != NULL && strcmp(filter,
> > > > service) == 0) || 
> > > > +  filter == NULL) {
> > > > +  services = reallocarray(services ,i+1
> > > > ,sizeof (char *));
> > > > +  if (services != NULL)
> > > > +  services[i++] = strdup(service);
> 
> In this block, I would find the !filter way a lot easier to read.

Agreed

> 
> > > 
> > > Technically you could check whether strdup() was successful, but
> > > I
> > > would accept this version.
> > 
> > Indeed, I forgot. As you gave enough comments to need a new version
> > of
> > the patchset, I'll include this too anyway. :-)
> > 
> > > 
> > > > +  else 
> > > > +  break;
> > > > +  }
> > > > +  service = strtok(NULL, service_delim);
> > > > +  }
> > > > +  }
> > > > +  }
> > > > +  }
> > > > +
> > > > +  free(line);
> > > 
> > > Are you sure that line will always be non-NULL?
> > > 
> > > What happens if you read an empty file and getline() exists
> > > immediately without ever allocating a buffer?
> > 
> > You are right. Not sure what will happen then. So I'll check if
> > 'line'
> > is not NULL before trying to free it.
> 
> I always do that (pretty much) and I am sure the compiler optimises
> that out in most cases.
> 
> > 
> > > 
> > > > +  fclose(fp);
> > > > +  }  else {
> > > > +  snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not
> > > > found.\n\n%s", addon, usage);
> > > > +  }
> > > > +
> > > > +  free(metafile);
> > > > +  *servicescnt = i;
> > > > +  return services;
> > > > +}
> > > > +
> > > > +// Calls initscript <service> with parameter <action>
> > > > +int initscript_action(const char *service, const char *action)
> > > > {
> > > > +  const char *initd_path = "/etc/rc.d/init.d";
> > > > +  char *initscript;
> > > 
> > > Same here. Please initialise it.
> > > 
> > > > +  char *argv[] = {
> > > > +  action,
> > > > +  NULL
> > > > +  };
> > > > +  int r = 0;
> > > > +
> > > > +  if ((r = asprintf(&initscript, "%s/%s", initd_path,
> > > > service))
> > > > != -1) {
> > > > +  r = run(initscript, argv);
> > > > +  free(initscript);
> > > > +  } else {
> > > > +  errno = ENOMEM;
> > > > +  }
> > > 
> > > This could look slightly cleaner as:
> > > 
> > > …
> > > 
> > > r = asprintf(&initscript, ….);
> > > if (r > 0) {
> > > r = run(initscript, argv);
> > > }
> > > 
> > > if (initscript)
> > > free(initscript);
> > > 
> > > return r;
> > > 
> > > Maybe… I don’t know.
> > 
> > Yes, indeed, as I don't have to set ENOMEM
> > 
> > > 
> > > > +
> > > > +  return r; 
> > > > +}
> > > > +
> > > > +// Move an initscript with filepattern from <src_path> to
> > > > <dest_path>
> > > > +// Returns:
> > > > +//  -1: Error during move or memory allocation. Details in
> > > > errno
> > > > +//  0: Success
> > > > +//  1: file was not moved, but is already in <dest_path>
> > > > +//  2: file does not exist in either in <src_path> or
> > > > <dest_path>
> > > > +int move_initscript_by_pattern(const char *src_path, const
> > > > char
> > > > *dest_path, const char *filepattern) {
> > > > +  char *src = NULL;
> > > > +  char *dest = NULL;
> > > > +  int r = 1;
> > > > +  char *filename = NULL;
> > > > +
> > > > +  if ((filename = find_file_in_dir(src_path, filepattern)) !=
> > > > NULL) {
> > > > +  if ((r = asprintf(&src, "%s/%s", src_path, filename)) != -
> > > > 1 &&
> > > > +  (r = asprintf(&dest, "%s/%s", dest_path, filename) !=
> > > > -1)) {
> > > > +  // move initscript
> > > > +  r = rename(src, dest);
> > > > +  } else {
> > > > +  errno = ENOMEM;
> > > > +  }
> > > > +
> > > > +  if (src != NULL)
> > > > +  free(src);
> > > > +  if (dest != NULL)
> > > > +  free(dest);
> > > > +  } else {
> > > > +  if ((filename = find_file_in_dir(dest_path, filepattern))
> > > > == NULL) 
> > > > +  r = 2;
> > > > +  }
> > > > +
> > > > +  if (filename != NULL)
> > > > +  free(filename);
> > > > +
> > > > +  return r;
> > > > +}
> > > > +
> > > > +// Enable/Disable addon service(s) by moving initscript
> > > > symlink
> > > > from/to disabled_path
> > > > +int toggle_service(const char *service, const char *action) {
> > > > +  const char *src_path, *dest_path;
> > > > +  char *filepattern; 
> > > 
> > > Init to NULL.
> > > 
> > > > +  int r = 0;
> > > > + 
> > > > +  if (asprintf(&filepattern, "S??%s", service) == -1) {
> > > > +  errno = ENOMEM;
> > > > +  return -1;
> > > > +  }
> > > > +
> > > > +  if (strcmp(action, "enable") == 0) {
> > > > +  src_path = disabled_path;
> > > > +  dest_path = enabled_path;
> > > > +  } else {
> > > > +  src_path = enabled_path;
> > > > +  dest_path = disabled_path;
> > > > +  }
> > > > +
> > > > +  // Ensure disabled_path exists
> > > > +  errno = 0;
> > > > +  if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP +
> > > > S_IROTH
> > > > + S_IXOTH) == -1 && errno != EEXIST) {
> > > > +  r = 1;
> > > > +  snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s.
> > > > (Error: %d)", disabled_path, errno);
> > > > +  } else {
> > > > +  r = move_initscript_by_pattern(src_path, dest_path,
> > > > filepattern);
> > > > +  if (r == -1 ) {
> > > > +  r = 1;
> > > > +  snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s.
> > > > (Error: %d)", action, service, errno);
> > > > +  } else if (r == 1) {
> > > > +  snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is
> > > > already %sd. Skipping...", service, action);
> > > > +  } else if (r == 2) {
> > > > +  snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s
> > > > service %s. (Service has no valid symlink in %s).", action,
> > > > service, src_path);
> > > > +  }
> > > > +  }
> > > > +
> > > > +  free(filepattern);
> > > > + 
> > > > +  return r;
> > > > +}
> > > > +
> > > > +// Print to stdout wether <service> is enabled or disabled on
> > > > boot
> > > > +// Prints <service> as Not available when initscript is not
> > > > found
> > > > +// in either enabled_path or disabled_path.
> > > > +void print_boot_status(char *service) {
> > > > +  char *filepattern;
> > > 
> > > NULL. How does this even run without it?
> > > 
> > > > +  if (asprintf(&filepattern, "S??%s", service) == -1) {
> > > > +  errno = ENOMEM;
> > > > +  return;
> > > > +  }
> > > > +
> > > > +  if (find_file_in_dir(enabled_path, filepattern) != NULL) 
> > > > +  fprintf(stdout, "%s is enabled on boot.\n", service);
> > > > +  else if (find_file_in_dir(disabled_path, filepattern) !=
> > > > NULL)

I started doubting this piece of code: find_file_in_dir returns a
malloc'ed filename. Which in this case will never be free'd, I think?,
at least not by my code.
I assume it is better to catch the return value in a char *filename and
then free it?

> > > > +  fprintf(stdout, "%s is disabled on boot.\n", service);
> > > > +  else
> > > > +  fprintf(stdout, "%s is not available for boot. (Service
> > > > has no valid symlink in either %s or %s).\n", service,
> > > > enabled_path, disabled_path);
> > > > +
> > > > +  free(filepattern);
> > > > +}
> > > > +
> > > > int main(int argc, char *argv[]) {
> > > > -  char command[BUFFER_SIZE];
> > > > -
> > > > -  if (!(initsetuid()))
> > > > -  exit(1);
> > > > -
> > > > -  if (argc < 3) {
> > > > -  fprintf(stderr, "\nMissing arguments.\n\naddonctrl
> > > > addon (start|stop|restart|reload|enable|disable)\n\n");
> > > > -  exit(1);
> > > > -  }
> > > > -
> > > > -  const char* name = argv[1];
> > > > -
> > > > -  if (strlen(name) > 32) {
> > > > -  fprintf(stderr, "\nString to large.\n\naddonctrl addon
> > > > (start|stop|restart|reload|enable|disable)\n\n");
> > > > -  exit(1);
> > > > -  }
> > > > -
> > > > -  // Check if the input argument is valid
> > > > -  if (!is_valid_argument_alnum(name)) {
> > > > -  fprintf(stderr, "Invalid add-on name: %s\n", name);
> > > > -  exit(2);
> > > > -  }
> > > > -
> > > > -  sprintf(command, "/opt/pakfire/db/installed/meta-%s",
> > > > name);
> > > > -  FILE *fp = fopen(command,"r");
> > > > -  if ( fp ) {
> > > > -  fclose(fp);
> > > > -  } else {
> > > > -  fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl
> > > > addon (start|stop|restart|reload|status|enable|disable)\n\n",
> > > > name);
> > > > -  exit(1);
> > > > -  }
> > > > -
> > > > -  if (strcmp(argv[2], "start") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1,
> > > > "/etc/rc.d/init.d/%s start", name);
> > > > -  safe_system(command);
> > > > -  } else if (strcmp(argv[2], "stop") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1,
> > > > "/etc/rc.d/init.d/%s stop", name);
> > > > -  safe_system(command);
> > > > -  } else if (strcmp(argv[2], "restart") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1,
> > > > "/etc/rc.d/init.d/%s restart", name);
> > > > -  safe_system(command);
> > > > -  } else if (strcmp(argv[2], "reload") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1,
> > > > "/etc/rc.d/init.d/%s reload", name);
> > > > -  safe_system(command);
> > > > -  } else if (strcmp(argv[2], "status") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1,
> > > > "/etc/rc.d/init.d/%s status", name);
> > > > -  safe_system(command);
> > > > -  } else if (strcmp(argv[2], "enable") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1, "mv -f
> > > > /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name);
> > > > -  safe_system(command);
> > > > -  } else if (strcmp(argv[2], "disable") == 0) {
> > > > -  snprintf(command, BUFFER_SIZE - 1, "mkdir -p
> > > > /etc/rc.d/rc3.d/off");
> > > > -  safe_system(command);
> > > > -  snprintf(command, BUFFER_SIZE - 1, "mv -f
> > > > /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name);
> > > > -  safe_system(command);
> > > > -  } else {
> > > > -  fprintf(stderr, "\nBad argument given.\n\naddonctrl
> > > > addon (start|stop|restart|reload|enable|disable)\n\n");
> > > > -  exit(1);
> > > > -  }
> > > > -
> > > > -  return 0;
> > > > +  char **services = NULL;
> > > > +  int servicescnt = 0;
> > > > +  char *addon = argv[1];
> > > > +  char *action = argv[2];
> > > > +  char *service_filter = NULL;
> > > > +  int r = 0;
> > > > +
> > > > +  if (!(initsetuid()))
> > > > +  exit(1);
> > > > +
> > > > +  if (argc < 3) {
> > > > +  fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage);
> > > > +  exit(1);
> > > > +  }
> > > > +
> > > > +  if (argc == 4 && strcmp(action, "list-services") != 0)
> > > > +  service_filter = argv[3];
> > > > +
> > > > +  if (strlen(addon) > 32) {
> > > > +  fprintf(stderr, "\nString too large.\n\n%s\n\n", usage);
> > > > +  exit(1);
> > > > +  }
> > > > +
> > > > +  // Check if the input argument is valid
> > > > +  if (!is_valid_argument_alnum(addon)) {
> > > > +  fprintf(stderr, "Invalid add-on name: %s.\n", addon);
> > > > +  exit(2);
> > > > +  }
> > > > +
> > > > +  // Get initscript name(s) from addon metadata
> > > > +  errno = 0;
> > > 
> > > There is usually no need to reset this.
> > 
> > I noticed before, during testing, that, since I do an mkdir in
> > toggle_service, but ignore the error if it is EEXIST, that later in
> > the
> > code after using other functions that could produce errno, errno
> > keeps
> > the value EEXIST if those functions just succeed.
> 
> Yes, you cannot rely on errno alone. The man pages for all those
> functions that might set errno will always say something like “will
> return a non-zero value and set errno accordingly”. So you will
> always have to check the return value first and find some more detail
> about the error in errno.
> 
> > And as get_addon_services could return NULL when there are no
> > services
> > but could also return NULL due to an error with errno set.
> 
> Yes, this is kind of a flaw in the system of using errno. I don’t
> know anything better though.
> 
> > However when no error occurred and the EEXIST error is still in
> > errno,
> > the code would falsely conclude that services is NULL due to an
> > error.
> > So now for safety, I make sure it is set to 0 before I call
> > get_addon_services.
> 
> Better reset in get_addon_services then.

I found this cert advisory write-up recommending always settings errno
to 0 before a library call: 
https://wiki.sei.cmu.edu/confluence/display/c/ERR30-
.+Take+care+when+reading+errno
I'll do it in get_addon_services, right before the library calls that
could set errno.

> 
> > 
> > > 
> > > > +  services = get_addon_services(addon, &servicescnt,
> > > > service_filter);
> > > > +  if (services == NULL || *services == 0) {
> > > > +  if (errno != 0)
> > > 
> > > This is not reliable error checking. The function should return a
> > > defined value if an error has occurred (e.g. NULL) and an error
> > > is
> > > detected, that error should be handled.
> > > 
> > > So in this case:
> > > 
> > > services = get_addon_services(…);
> > > if (!services) {
> > > printf(“ERROR: …\”);
> > > 
> > > }
> > > 
> > > … Other code, on success…
> > > 
> > > Now, NULL is not a very good indicator because your problem might
> > > be
> > > that an error has been found and therefore the function aborted,
> > > or
> > > that there is actually no services available for this package.
> > > 
> > > You could handle this by setting errno to something like ENOENT
> > > (No
> > > such file or directory) and handle that separately:
> > > 
> > > services = get_addon_services(…);
> > > if (!services) {
> > > switch (errno) {
> > > case ENOENT:
> > > DO SOMETHING;
> > > Break;
> > > 
> > > default:
> > > printf(“ERROR: …\”);
> > > DO SOMETING ELSE
> > > }
> > 
> > I'm not sure why it is not reliable?
> 
> Reliable in that sense, that the function has the same return value
> for two different cases.
> 
> Here, it might not play the biggest role, but if some other software
> would call this function (because it was part of some library), the
> caller might get confused.

After reading above mentioned cert advisory, I think I clearly
understand why this code is indeed not reliable :-). At least it is not
'compliant' to that advisory.

> 
> > I had thought about setting errno before; but I saw it being
> > strongly
> > discouraged in many discussions on the web; unless you where
> > writing an
> > actual system library.
> 
> Really? And what do they suggest using instead?

That varies :-) 
Most common suggestion is the non-helpful "use another mechanism to
report errors"

Some suggest to return an error of type int * or size_t * as an output
parameter, or returning a negative value assuming the return type is a
signed type eventually with:
enum myerrors {
    ERR_NO_MEMORY    = -1,
    ERR_BAD_ARGS     = -2,
    ERR_CPU_EXPLODED = -3,
    // and so on
};
for clarity.

Some even suggest implementing a simulation of the C standard errno
mechanism:
/* your_errno.c */
__thread int g_your_error_code;

/* your_errno.h */
extern __thread int g_your_error_code
#define set_your_errno(err) (g_your_error_code = (err))
#define your_errno (g_your_error_code)

Also some refer to this cert advisory as implicit recommendation to not
set errno for non system-library functions, but rather make the
function return an errno using return type errno_t:
https://wiki.sei.cmu.edu/confluence/display/c/DCL09-C.+Declare+functions+that+return+errno+with+a+return+type+of+errno_t

But I think I will go with an output parameter indicating success or
error, and when error the value can indicate whether it is a system
error or another error. So depending on the value of that returned
error, check errno or display a more suitable error message.

(I tried before to have the function return an int indicating success
or error, but I then failed to reliably pass the array back in an
output parameter. So in the end I went with returning the array.)

> 
> > So I went for my own 'errormsg'; And I check if
> > errno is set for system errors, if not, check my own errormsg for
> > function errors. If both are clear, services is NULL due to no
> > services. (which could then be due to an invalid filter, or just no
> > services for the addon)
> > 
> > > 
> > > Finally, instead of printing the errno number in the error
> > > message,
> > > you can use %m (without any argument) which will be automatically
> > > converted into something human readable.
> > 
> > Are you sure? As I found this:
> > > The %m directive is not portable, use %s mapped to an argument of
> > > strerror(errno) (or a version of strerror_r) instead. 
> > here:
> > https://www.gnu.org/software/gnulib/manual/html_node/asprintf.html
> 
> We are only using glibc here.
> 
> As far as I know, glibc, musl, all the BSDs support it. What else do
> we need?
> 
> This binary won’t be POSIX compatible, indeed.

Ok. I was under the impression that the document spoke about glibc but
as I understand it now, it is about gnulib :-)

I should be able to produce a new version of this patchset by Tuesday
evening.

Regards
Robin
> 
> > 
> > > 
> > > > +  fprintf(stderr, "\nSystem error occured. (Error:
> > > > %d)\n\n", errno);
> > > > +  else if (strcmp(errormsg, "") != 0)
> > > > +  fprintf(stderr, "\n%s\n\n", errormsg);
> > > > +  else if (service_filter != NULL)
> > > > +  fprintf(stderr, "\nNo service '%s' found for addon
> > > > '%s'. Use 'list-services' to get a list of available
> > > > services\n\n%s\n\n", service_filter, addon, usage);
> > > > +  else
> > > > +  fprintf(stderr, "\nAddon '%s' has no services.\n\n",
> > > > addon);
> > > > +  exit(1);
> > > > +  }
> > > > +
> > > > +  // Handle requested action
> > > > +  if (strcmp(action, "start") == 0 ||
> > > > +  strcmp(action, "stop") == 0 ||
> > > > +  strcmp(action, "restart") == 0 ||
> > > > +  strcmp(action, "reload") == 0 ||
> > > > +  strcmp(action, "status") == 0) {
> > > > +
> > > > +  errno = 0;
> > > > +  for(int i = 0; i < servicescnt; i++) {
> > > > +  if (initscript_action(services[i], action) != 0) {
> > > > +  r = 1;
> > > > +  if (errno != 0) 
> > > > +  fprintf(stderr, "\nSystem error occured.
> > > > (Error: %d)\n\n", errno);
> > > > +  break;
> > > > +  }
> > > > +  }
> > > > +
> > > > +  } else if (strcmp(action, "enable") == 0 ||
> > > > +  strcmp(action, "disable") == 0) {
> > > > +
> > > > +  errno = 0;
> > > > +  for(int i = 0; i < servicescnt; i++) {
> > > > +  if (toggle_service(services[i], action) == 0) {
> > > > +  fprintf(stdout, "%sd service %s\n", action,
> > > > services[i]);
> > > 
> > > If you want to just print something, just use printf.
> > 
> > Ok
> > 
> > > 
> > > > +  } else if (errno != 0) {
> > > > +  r = 1;
> > > > +  fprintf(stderr, "\nSystem error occured. (Error:
> > > > %d)\n\n", errno);
> > > > +  break;
> > > > +  } else {
> > > > +  r = 1;
> > > > +  fprintf(stderr, "\n%s\n\n", errormsg);
> > > > +  }
> > > > +  }
> > > > +
> > > > +  } else if (strcmp(action, "boot-status") == 0) {
> > > > +  errno = 0;
> > > > +  for(int i = 0; i < servicescnt; i++) {
> > > > +  print_boot_status(services[i]);
> > > > +  if (errno != 0) {
> > > > +  r = 1;
> > > > +  fprintf(stderr, "\nSystem error occured. (Error:
> > > > %d)\n\n", errno);
> > > > +  break;
> > > > +  }
> > > > +  }
> > > > + 
> > > > +  } else if (strcmp(action, "list-services") == 0) {
> > > > +  fprintf(stdout, "\nServices for addon %s:\n", addon);
> > > > +  for(int i = 0; i < servicescnt; i++) {
> > > > +  fprintf(stdout, "  %s\n", services[i]);
> > > > +  }
> > > > +  fprintf(stdout, "\n");
> > > > +
> > > > +  } else {
> > > > +  fprintf(stderr, "\nBad argument given.\n\n%s\n\n", usage);
> > > > +  r = 1;
> > > > +  }
> > > > +
> > > > +  // Cleanup
> > > > +  for(int i = 0; i < servicescnt; i++) 
> > > > +  free(services[i]);
> > > > +  free(services);
> > > > +
> > > > +  return r;
> > > > }
> > > 
> > > Otherwise, this looks a lot better since the last version :)
> > 
> > Thanks. I'm learning a lot.
> 
> Happy to help :)
> 
> > 
> > Robin
> > 
> > > 
> > > -Michael

-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.