From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Roevens To: development@lists.ipfire.org Subject: Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata Date: Mon, 10 Oct 2022 14:27:23 +0200 Message-ID: <9ea70b6a11583c8258b71bdc5406cdd14440af3a.camel@sicho.home> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8951957924080537621==" List-Id: --===============8951957924080537621== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Michael Tremer schreef op ma 10-10-2022 om 11:04 [+0100]: > Hello, >=20 > > On 9 Oct 2022, at 00:09, Robin Roevens > > wrote: > >=20 > > Hi Michael > >=20 > > Thanks again for the review and comments. > >=20 > > Michael Tremer schreef op vr 07-10-2022 om 16:01 [+0100]: > > > Hello again, > > >=20 > > > > On 6 Oct 2022, at 18:59, Robin Roevens > > > > > > > > wrote: > > > >=20 > > > > * 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. > > > >=20 > > > > Fixes: Bug#12935 > > > > Signed-off-by: Robin Roevens > > > > --- > > > > src/misc-progs/addonctrl.c | 397 > > > > +++++++++++++++++++++++++++++++--- > > > > --- > > > > 1 file changed, 336 insertions(+), 61 deletions(-) > > > >=20 > > > > 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 > > > > #include > > > > #include > > > > +#include > > > > #include > > > > +#include > > > > +#include > > > > +#include > > > > #include "setuid.h" > > > >=20 > > > > #define BUFFER_SIZE 1024 > > > >=20 > > > > +const char *enabled_path =3D "/etc/rc.d/rc3.d"; > > > > +const char *disabled_path =3D "/etc/rc.d/rc3.d/off"; > > > > + > > > > +char errormsg[BUFFER_SIZE] =3D ""; > > > > +const char *usage =3D=20 > > > > +=C2=A0 "Usage\n" > > > > +=C2=A0 "=C2=A0 addonctrl > > > > (start|stop|restart|reload|enable|disable|status|boot- > > > > status|list- > > > > services) []\n" > > > > +=C2=A0 "\n" > > > > +=C2=A0 "Options:\n" > > > > +=C2=A0 "=C2=A0 \t\tName of the addon to control\n" > > > > +=C2=A0 "=C2=A0 \t\tSpecific service of the addon to control > > > > (optional)\n" > > > > +=C2=A0 "=C2=A0 \t\t\tBy default the requested action is performed on= all > > > > related services. See also 'list-services'.\n" > > > > +=C2=A0 "=C2=A0 start\t\t\tStart service(s) of the addon\n" > > > > +=C2=A0 "=C2=A0 stop\t\t\tStop service(s) of the addon\n" > > > > +=C2=A0 "=C2=A0 restart\t\tRestart service(s) of the addon\n" > > > > +=C2=A0 "=C2=A0 enable\t\tEnable service(s) of the addon to start at > > > > boot\n" > > > > +=C2=A0 "=C2=A0 disable\t\tDisable service(s) of the addon to start at > > > > boot\n" > > > > +=C2=A0 "=C2=A0 status\t\tDisplay current state of addon service(s)\n" > > > > +=C2=A0 "=C2=A0 boot-status\t\tDisplay wether service(s) is enabled on > > > > boot > > > > or not\n" > > > > +=C2=A0 "=C2=A0 list-services\t\tDisplay a list of services related to > > > > the > > > > addon"; > > > > + > > > > +// Find a file using as glob pattern.=20 > > > > +// Returns the found filename or NULL if not found > > > > +char *find_file_in_dir(const char *path, const char > > > > *filepattern)=20 > > > > +{ > > > > +=C2=A0 struct dirent *entry; > > > > +=C2=A0 DIR *dp; > > > > +=C2=A0 char *found =3D NULL; > > > > + > > > > +=C2=A0 if ((dp =3D opendir(path)) !=3D NULL) { > > > > +=C2=A0 while(found =3D=3D NULL && (entry =3D readdir(dp)) !=3D NULL) > > > > +=C2=A0 if (fnmatch(filepattern, entry->d_name, FNM_PATHNAME) > > > > =3D=3D 0) > > > > +=C2=A0 found =3D strdup(entry->d_name); > > > > + > > > > +=C2=A0 closedir(dp); > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 return found; > > > > +} > > > > + > > > > +// Reads Services metadata for . > > > > +// Returns pointer to array of strings containing the services > > > > for > > > > =20 > > > > +// and sets to the number of found services > > > > +char **get_addon_services(const char *addon, int *servicescnt, > > > > const char *filter) { > > > > +=C2=A0 const char *metafile_prefix =3D > > > > "/opt/pakfire/db/installed/meta- > > > > "; > > > > +=C2=A0 const char *metadata_key =3D "Services"; > > > > +=C2=A0 const char *keyvalue_delim =3D ":"; > > > > +=C2=A0 const char *service_delim =3D " "; > > > > +=C2=A0 char *token; > > > > +=C2=A0 char **services =3D NULL; > > > > +=C2=A0 char *service; > > > > +=C2=A0 char *line =3D NULL; > > > > +=C2=A0 size_t line_len =3D 0; > > > > +=C2=A0 int i =3D 0; > > > > +=C2=A0 char *metafile; > > > > +=20 > > > > +=C2=A0 if (addon =3D=3D NULL) { > > > > +=C2=A0 errno =3D EINVAL; > > > > +=C2=A0 return NULL; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 if (asprintf(&metafile, "%s%s", metafile_prefix, addon) =3D= =3D - > > > > 1) > > > > { > > > > +=C2=A0 errno =3D ENOMEM; > > > > +=C2=A0 return NULL; > > > > +=C2=A0 } > > >=20 > > > You should initialise metafile with NULL before passing it to > > > asprintf(). > >=20 > > 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. > >=20 > > 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. >=20 > I suppose my concern is rooted in the different implementations of > such functions. >=20 > 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. >=20 > Should this be absolutely unnecessary, the compiler will skip the > initialisation anyways. >=20 > > 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. > >=20 > > But I'll initialize them. It's probably indeed the safest to do > > anyway. >=20 > Yes, better be safe than sorry. >=20 > reallocarray() would require this, and so it would be more uniform to > initialise everything. >=20 > >=20 > > >=20 > > > asprintf() will automatically set errno, actually. > >=20 > > 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. >=20 > Depending on implementation, asprintf might never set it itself, but > the function it calls (i.e. malloc, etc.). >=20 > > And I have investigated the source of asprintf, and it does not set > > errno when there is a problem with malloc..=20 > > 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. > >=20 > > >=20 > > > > + > > > > +=C2=A0 FILE *fp =3D fopen(metafile,"r"); > > > > +=C2=A0 if (fp !=3D NULL) { > > >=20 > > > Just FYI, I like writing this check as =E2=80=9Cif (!fp)=E2=80=9D becau= se it is a > > > a > > > lot shorter... > >=20 > > 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. >=20 > Yes, there are indeed two camps. One of them is being so funny that > they set NULL to a value like =E2=80=9C5=E2=80=9D and check if your code st= ill works. >=20 > My code won=E2=80=99t. But I am sure their code won=E2=80=99t work any more= if I > define 5 as 6, and the number 2 because 9 and free becomes malloc. > Changing arbitrary things doesn=E2=80=99t make any sense. >=20 > > But if you think that is unnecessary I'll change it. >=20 > 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. >=20 > > Should I change all my other checks against NULL also? Or only the > > filepointer ?=20 >=20 > If you don=E2=80=99t 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 :-) >=20 > >=20 > > >=20 > > > > +=C2=A0 // Get initscript(s) for addon from meta-file > > > > +=C2=A0 while (getline(&line, &line_len, fp) !=3D -1 && services =3D= =3D > > > > NULL) { > > > > +=C2=A0 // Strip newline > > > > +=C2=A0 char *newline =3D strchr(line, '\n'); > > > > +=C2=A0 if (newline) *newline =3D 0; > > >=20 > > > Yes, this will cut off the trailing newline. > > >=20 > > > > + > > > > +=C2=A0 // Parse key/value and look for required key. > > > > +=C2=A0 token =3D strtok(line, keyvalue_delim); > > > > +=C2=A0 if (token !=3D NULL && strcmp(token, metadata_key) =3D=3D 0) > > > > { > > > > +=C2=A0 token =3D strtok(NULL, keyvalue_delim); > > > > +=C2=A0 if (token !=3D NULL) { > > > > +=C2=A0 // Put each service in services array > > > > +=C2=A0 service =3D strtok(token, service_delim); > > > > +=C2=A0 while (service !=3D NULL) { > > > > +=C2=A0 // if filter is set, only select filtered > > > > service > > > > +=C2=A0 if ((filter !=3D NULL && strcmp(filter, > > > > service) =3D=3D 0) ||=20 > > > > +=C2=A0 filter =3D=3D NULL) { > > > > +=C2=A0 services =3D reallocarray(services ,i+1 > > > > ,sizeof (char *)); > > > > +=C2=A0 if (services !=3D NULL) > > > > +=C2=A0 services[i++] =3D strdup(service); >=20 > In this block, I would find the !filter way a lot easier to read. Agreed >=20 > > >=20 > > > Technically you could check whether strdup() was successful, but > > > I > > > would accept this version. > >=20 > > Indeed, I forgot. As you gave enough comments to need a new version > > of > > the patchset, I'll include this too anyway. :-) > >=20 > > >=20 > > > > +=C2=A0 else=20 > > > > +=C2=A0 break; > > > > +=C2=A0 } > > > > +=C2=A0 service =3D strtok(NULL, service_delim); > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 free(line); > > >=20 > > > Are you sure that line will always be non-NULL? > > >=20 > > > What happens if you read an empty file and getline() exists > > > immediately without ever allocating a buffer? > >=20 > > You are right. Not sure what will happen then. So I'll check if > > 'line' > > is not NULL before trying to free it. >=20 > I always do that (pretty much) and I am sure the compiler optimises > that out in most cases. >=20 > >=20 > > >=20 > > > > +=C2=A0 fclose(fp); > > > > +=C2=A0 }=C2=A0 else { > > > > +=C2=A0 snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not > > > > found.\n\n%s", addon, usage); > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 free(metafile); > > > > +=C2=A0 *servicescnt =3D i; > > > > +=C2=A0 return services; > > > > +} > > > > + > > > > +// Calls initscript with parameter > > > > +int initscript_action(const char *service, const char *action) > > > > { > > > > +=C2=A0 const char *initd_path =3D "/etc/rc.d/init.d"; > > > > +=C2=A0 char *initscript; > > >=20 > > > Same here. Please initialise it. > > >=20 > > > > +=C2=A0 char *argv[] =3D { > > > > +=C2=A0 action, > > > > +=C2=A0 NULL > > > > +=C2=A0 }; > > > > +=C2=A0 int r =3D 0; > > > > + > > > > +=C2=A0 if ((r =3D asprintf(&initscript, "%s/%s", initd_path, > > > > service)) > > > > !=3D -1) { > > > > +=C2=A0 r =3D run(initscript, argv); > > > > +=C2=A0 free(initscript); > > > > +=C2=A0 } else { > > > > +=C2=A0 errno =3D ENOMEM; > > > > +=C2=A0 } > > >=20 > > > This could look slightly cleaner as: > > >=20 > > > =E2=80=A6 > > >=20 > > > r =3D asprintf(&initscript, =E2=80=A6.); > > > if (r > 0) { > > > r =3D run(initscript, argv); > > > } > > >=20 > > > if (initscript) > > > free(initscript); > > >=20 > > > return r; > > >=20 > > > Maybe=E2=80=A6 I don=E2=80=99t know. > >=20 > > Yes, indeed, as I don't have to set ENOMEM > >=20 > > >=20 > > > > + > > > > +=C2=A0 return r;=20 > > > > +} > > > > + > > > > +// Move an initscript with filepattern from to > > > > > > > > +// Returns: > > > > +//=C2=A0 -1: Error during move or memory allocation. Details in > > > > errno > > > > +//=C2=A0 0: Success > > > > +//=C2=A0 1: file was not moved, but is already in > > > > +//=C2=A0 2: file does not exist in either in or > > > > > > > > +int move_initscript_by_pattern(const char *src_path, const > > > > char > > > > *dest_path, const char *filepattern) { > > > > +=C2=A0 char *src =3D NULL; > > > > +=C2=A0 char *dest =3D NULL; > > > > +=C2=A0 int r =3D 1; > > > > +=C2=A0 char *filename =3D NULL; > > > > + > > > > +=C2=A0 if ((filename =3D find_file_in_dir(src_path, filepattern)) != =3D > > > > NULL) { > > > > +=C2=A0 if ((r =3D asprintf(&src, "%s/%s", src_path, filename)) !=3D - > > > > 1 && > > > > +=C2=A0 (r =3D asprintf(&dest, "%s/%s", dest_path, filename) !=3D > > > > -1)) { > > > > +=C2=A0 // move initscript > > > > +=C2=A0 r =3D rename(src, dest); > > > > +=C2=A0 } else { > > > > +=C2=A0 errno =3D ENOMEM; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 if (src !=3D NULL) > > > > +=C2=A0 free(src); > > > > +=C2=A0 if (dest !=3D NULL) > > > > +=C2=A0 free(dest); > > > > +=C2=A0 } else { > > > > +=C2=A0 if ((filename =3D find_file_in_dir(dest_path, filepattern)) > > > > =3D=3D NULL)=20 > > > > +=C2=A0 r =3D 2; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 if (filename !=3D NULL) > > > > +=C2=A0 free(filename); > > > > + > > > > +=C2=A0 return r; > > > > +} > > > > + > > > > +// Enable/Disable addon service(s) by moving initscript > > > > symlink > > > > from/to disabled_path > > > > +int toggle_service(const char *service, const char *action) { > > > > +=C2=A0 const char *src_path, *dest_path; > > > > +=C2=A0 char *filepattern;=20 > > >=20 > > > Init to NULL. > > >=20 > > > > +=C2=A0 int r =3D 0; > > > > +=20 > > > > +=C2=A0 if (asprintf(&filepattern, "S??%s", service) =3D=3D -1) { > > > > +=C2=A0 errno =3D ENOMEM; > > > > +=C2=A0 return -1; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 if (strcmp(action, "enable") =3D=3D 0) { > > > > +=C2=A0 src_path =3D disabled_path; > > > > +=C2=A0 dest_path =3D enabled_path; > > > > +=C2=A0 } else { > > > > +=C2=A0 src_path =3D enabled_path; > > > > +=C2=A0 dest_path =3D disabled_path; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 // Ensure disabled_path exists > > > > +=C2=A0 errno =3D 0; > > > > +=C2=A0 if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP + > > > > S_IROTH > > > > + S_IXOTH) =3D=3D -1 && errno !=3D EEXIST) { > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s. > > > > (Error: %d)", disabled_path, errno); > > > > +=C2=A0 } else { > > > > +=C2=A0 r =3D move_initscript_by_pattern(src_path, dest_path, > > > > filepattern); > > > > +=C2=A0 if (r =3D=3D -1 ) { > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s. > > > > (Error: %d)", action, service, errno); > > > > +=C2=A0 } else if (r =3D=3D 1) { > > > > +=C2=A0 snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is > > > > already %sd. Skipping...", service, action); > > > > +=C2=A0 } else if (r =3D=3D 2) { > > > > +=C2=A0 snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s > > > > service %s. (Service has no valid symlink in %s).", action, > > > > service, src_path); > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 free(filepattern); > > > > +=20 > > > > +=C2=A0 return r; > > > > +} > > > > + > > > > +// Print to stdout wether is enabled or disabled on > > > > boot > > > > +// Prints as Not available when initscript is not > > > > found > > > > +// in either enabled_path or disabled_path. > > > > +void print_boot_status(char *service) { > > > > +=C2=A0 char *filepattern; > > >=20 > > > NULL. How does this even run without it? > > >=20 > > > > +=C2=A0 if (asprintf(&filepattern, "S??%s", service) =3D=3D -1) { > > > > +=C2=A0 errno =3D ENOMEM; > > > > +=C2=A0 return; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 if (find_file_in_dir(enabled_path, filepattern) !=3D NULL)=20 > > > > +=C2=A0 fprintf(stdout, "%s is enabled on boot.\n", service); > > > > +=C2=A0 else if (find_file_in_dir(disabled_path, filepattern) !=3D > > > > 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? > > > > +=C2=A0 fprintf(stdout, "%s is disabled on boot.\n", service); > > > > +=C2=A0 else > > > > +=C2=A0 fprintf(stdout, "%s is not available for boot. (Service > > > > has no valid symlink in either %s or %s).\n", service, > > > > enabled_path, disabled_path); > > > > + > > > > +=C2=A0 free(filepattern); > > > > +} > > > > + > > > > int main(int argc, char *argv[]) { > > > > -=C2=A0 char command[BUFFER_SIZE]; > > > > - > > > > -=C2=A0 if (!(initsetuid())) > > > > -=C2=A0 exit(1); > > > > - > > > > -=C2=A0 if (argc < 3) { > > > > -=C2=A0 fprintf(stderr, "\nMissing arguments.\n\naddonctrl > > > > addon (start|stop|restart|reload|enable|disable)\n\n"); > > > > -=C2=A0 exit(1); > > > > -=C2=A0 } > > > > - > > > > -=C2=A0 const char* name =3D argv[1]; > > > > - > > > > -=C2=A0 if (strlen(name) > 32) { > > > > -=C2=A0 fprintf(stderr, "\nString to large.\n\naddonctrl addon > > > > (start|stop|restart|reload|enable|disable)\n\n"); > > > > -=C2=A0 exit(1); > > > > -=C2=A0 } > > > > - > > > > -=C2=A0 // Check if the input argument is valid > > > > -=C2=A0 if (!is_valid_argument_alnum(name)) { > > > > -=C2=A0 fprintf(stderr, "Invalid add-on name: %s\n", name); > > > > -=C2=A0 exit(2); > > > > -=C2=A0 } > > > > - > > > > -=C2=A0 sprintf(command, "/opt/pakfire/db/installed/meta-%s", > > > > name); > > > > -=C2=A0 FILE *fp =3D fopen(command,"r"); > > > > -=C2=A0 if ( fp ) { > > > > -=C2=A0 fclose(fp); > > > > -=C2=A0 } else { > > > > -=C2=A0 fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl > > > > addon (start|stop|restart|reload|status|enable|disable)\n\n", > > > > name); > > > > -=C2=A0 exit(1); > > > > -=C2=A0 } > > > > - > > > > -=C2=A0 if (strcmp(argv[2], "start") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, > > > > "/etc/rc.d/init.d/%s start", name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else if (strcmp(argv[2], "stop") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, > > > > "/etc/rc.d/init.d/%s stop", name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else if (strcmp(argv[2], "restart") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, > > > > "/etc/rc.d/init.d/%s restart", name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else if (strcmp(argv[2], "reload") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, > > > > "/etc/rc.d/init.d/%s reload", name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else if (strcmp(argv[2], "status") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, > > > > "/etc/rc.d/init.d/%s status", name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else if (strcmp(argv[2], "enable") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, "mv -f > > > > /etc/rc.d/rc3.d/off/S??%s /etc/rc.d/rc3.d" , name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else if (strcmp(argv[2], "disable") =3D=3D 0) { > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, "mkdir -p > > > > /etc/rc.d/rc3.d/off"); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 snprintf(command, BUFFER_SIZE - 1, "mv -f > > > > /etc/rc.d/rc3.d/S??%s /etc/rc.d/rc3.d/off" , name); > > > > -=C2=A0 safe_system(command); > > > > -=C2=A0 } else { > > > > -=C2=A0 fprintf(stderr, "\nBad argument given.\n\naddonctrl > > > > addon (start|stop|restart|reload|enable|disable)\n\n"); > > > > -=C2=A0 exit(1); > > > > -=C2=A0 } > > > > - > > > > -=C2=A0 return 0; > > > > +=C2=A0 char **services =3D NULL; > > > > +=C2=A0 int servicescnt =3D 0; > > > > +=C2=A0 char *addon =3D argv[1]; > > > > +=C2=A0 char *action =3D argv[2]; > > > > +=C2=A0 char *service_filter =3D NULL; > > > > +=C2=A0 int r =3D 0; > > > > + > > > > +=C2=A0 if (!(initsetuid())) > > > > +=C2=A0 exit(1); > > > > + > > > > +=C2=A0 if (argc < 3) { > > > > +=C2=A0 fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage); > > > > +=C2=A0 exit(1); > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 if (argc =3D=3D 4 && strcmp(action, "list-services") !=3D 0) > > > > +=C2=A0 service_filter =3D argv[3]; > > > > + > > > > +=C2=A0 if (strlen(addon) > 32) { > > > > +=C2=A0 fprintf(stderr, "\nString too large.\n\n%s\n\n", usage); > > > > +=C2=A0 exit(1); > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 // Check if the input argument is valid > > > > +=C2=A0 if (!is_valid_argument_alnum(addon)) { > > > > +=C2=A0 fprintf(stderr, "Invalid add-on name: %s.\n", addon); > > > > +=C2=A0 exit(2); > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 // Get initscript name(s) from addon metadata > > > > +=C2=A0 errno =3D 0; > > >=20 > > > There is usually no need to reset this. > >=20 > > 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. >=20 > Yes, you cannot rely on errno alone. The man pages for all those > functions that might set errno will always say something like =E2=80=9Cwill > return a non-zero value and set errno accordingly=E2=80=9D. So you will > always have to check the return value first and find some more detail > about the error in errno. >=20 > > 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. >=20 > Yes, this is kind of a flaw in the system of using errno. I don=E2=80=99t > know anything better though. >=20 > > 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. >=20 > Better reset in get_addon_services then. I found this cert advisory write-up recommending always settings errno to 0 before a library call:=C2=A0 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. >=20 > >=20 > > >=20 > > > > +=C2=A0 services =3D get_addon_services(addon, &servicescnt, > > > > service_filter); > > > > +=C2=A0 if (services =3D=3D NULL || *services =3D=3D 0) { > > > > +=C2=A0 if (errno !=3D 0) > > >=20 > > > 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. > > >=20 > > > So in this case: > > >=20 > > > services =3D get_addon_services(=E2=80=A6); > > > if (!services) { > > > printf(=E2=80=9CERROR: =E2=80=A6\=E2=80=9D); > > >=20 > > > } > > >=20 > > > =E2=80=A6 Other code, on success=E2=80=A6 > > >=20 > > > 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. > > >=20 > > > You could handle this by setting errno to something like ENOENT > > > (No > > > such file or directory) and handle that separately: > > >=20 > > > services =3D get_addon_services(=E2=80=A6); > > > if (!services) { > > > switch (errno) { > > > case ENOENT: > > > DO SOMETHING; > > > Break; > > >=20 > > > default: > > > printf(=E2=80=9CERROR: =E2=80=A6\=E2=80=9D); > > > DO SOMETING ELSE > > > } > >=20 > > I'm not sure why it is not reliable? >=20 > Reliable in that sense, that the function has the same return value > for two different cases. >=20 > 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. >=20 > > 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. >=20 > Really? And what do they suggest using instead? That varies :-)=C2=A0 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 =3D -1, ERR_BAD_ARGS =3D -2, ERR_CPU_EXPLODED =3D -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 =3D (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.) >=20 > > 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) > >=20 > > >=20 > > > 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. > >=20 > > 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.=20 > > here: > > https://www.gnu.org/software/gnulib/manual/html_node/asprintf.html >=20 > We are only using glibc here. >=20 > As far as I know, glibc, musl, all the BSDs support it. What else do > we need? >=20 > This binary won=E2=80=99t 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 >=20 > >=20 > > >=20 > > > > +=C2=A0 fprintf(stderr, "\nSystem error occured. (Error: > > > > %d)\n\n", errno); > > > > +=C2=A0 else if (strcmp(errormsg, "") !=3D 0) > > > > +=C2=A0 fprintf(stderr, "\n%s\n\n", errormsg); > > > > +=C2=A0 else if (service_filter !=3D NULL) > > > > +=C2=A0 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); > > > > +=C2=A0 else > > > > +=C2=A0 fprintf(stderr, "\nAddon '%s' has no services.\n\n", > > > > addon); > > > > +=C2=A0 exit(1); > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 // Handle requested action > > > > +=C2=A0 if (strcmp(action, "start") =3D=3D 0 || > > > > +=C2=A0 strcmp(action, "stop") =3D=3D 0 || > > > > +=C2=A0 strcmp(action, "restart") =3D=3D 0 || > > > > +=C2=A0 strcmp(action, "reload") =3D=3D 0 || > > > > +=C2=A0 strcmp(action, "status") =3D=3D 0) { > > > > + > > > > +=C2=A0 errno =3D 0; > > > > +=C2=A0 for(int i =3D 0; i < servicescnt; i++) { > > > > +=C2=A0 if (initscript_action(services[i], action) !=3D 0) { > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 if (errno !=3D 0)=20 > > > > +=C2=A0 fprintf(stderr, "\nSystem error occured. > > > > (Error: %d)\n\n", errno); > > > > +=C2=A0 break; > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 } else if (strcmp(action, "enable") =3D=3D 0 || > > > > +=C2=A0 strcmp(action, "disable") =3D=3D 0) { > > > > + > > > > +=C2=A0 errno =3D 0; > > > > +=C2=A0 for(int i =3D 0; i < servicescnt; i++) { > > > > +=C2=A0 if (toggle_service(services[i], action) =3D=3D 0) { > > > > +=C2=A0 fprintf(stdout, "%sd service %s\n", action, > > > > services[i]); > > >=20 > > > If you want to just print something, just use printf. > >=20 > > Ok > >=20 > > >=20 > > > > +=C2=A0 } else if (errno !=3D 0) { > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 fprintf(stderr, "\nSystem error occured. (Error: > > > > %d)\n\n", errno); > > > > +=C2=A0 break; > > > > +=C2=A0 } else { > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 fprintf(stderr, "\n%s\n\n", errormsg); > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 } else if (strcmp(action, "boot-status") =3D=3D 0) { > > > > +=C2=A0 errno =3D 0; > > > > +=C2=A0 for(int i =3D 0; i < servicescnt; i++) { > > > > +=C2=A0 print_boot_status(services[i]); > > > > +=C2=A0 if (errno !=3D 0) { > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 fprintf(stderr, "\nSystem error occured. (Error: > > > > %d)\n\n", errno); > > > > +=C2=A0 break; > > > > +=C2=A0 } > > > > +=C2=A0 } > > > > +=20 > > > > +=C2=A0 } else if (strcmp(action, "list-services") =3D=3D 0) { > > > > +=C2=A0 fprintf(stdout, "\nServices for addon %s:\n", addon); > > > > +=C2=A0 for(int i =3D 0; i < servicescnt; i++) { > > > > +=C2=A0 fprintf(stdout, "=C2=A0 %s\n", services[i]); > > > > +=C2=A0 } > > > > +=C2=A0 fprintf(stdout, "\n"); > > > > + > > > > +=C2=A0 } else { > > > > +=C2=A0 fprintf(stderr, "\nBad argument given.\n\n%s\n\n", usage); > > > > +=C2=A0 r =3D 1; > > > > +=C2=A0 } > > > > + > > > > +=C2=A0 // Cleanup > > > > +=C2=A0 for(int i =3D 0; i < servicescnt; i++)=20 > > > > +=C2=A0 free(services[i]); > > > > +=C2=A0 free(services); > > > > + > > > > +=C2=A0 return r; > > > > } > > >=20 > > > Otherwise, this looks a lot better since the last version :) > >=20 > > Thanks. I'm learning a lot. >=20 > Happy to help :) >=20 > >=20 > > Robin > >=20 > > >=20 > > > -Michael --=20 Dit bericht is gescanned op virussen en andere gevaarlijke inhoud door MailScanner en lijkt schoon te zijn. --===============8951957924080537621==--