From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer 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:36 +0100 Message-ID: <6020FCB2-57BB-4F6D-88F0-3D266C7CCBAB@ipfire.org> In-Reply-To: <9ea70b6a11583c8258b71bdc5406cdd14440af3a.camel@sicho.home> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5193840404465553484==" List-Id: --===============5193840404465553484== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello Robin, > On 10 Oct 2022, at 13:27, Robin Roevens wrote: >=20 > Hi >=20 > 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 >>>>> + "Usage\n" >>>>> + " addonctrl >>>>> (start|stop|restart|reload|enable|disable|status|boot- >>>>> status|list- >>>>> services) []\n" >>>>> + "\n" >>>>> + "Options:\n" >>>>> + " \t\tName of the addon to control\n" >>>>> + " \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 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 >>>>> +{ >>>>> + struct dirent *entry; >>>>> + DIR *dp; >>>>> + char *found =3D NULL; >>>>> + >>>>> + if ((dp =3D opendir(path)) !=3D NULL) { >>>>> + while(found =3D=3D NULL && (entry =3D readdir(dp)) !=3D NULL) >>>>> + if (fnmatch(filepattern, entry->d_name, FNM_PATHNAME) >>>>> =3D=3D 0) >>>>> + found =3D strdup(entry->d_name); >>>>> + >>>>> + closedir(dp); >>>>> + } >>>>> + >>>>> + 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) { >>>>> + const char *metafile_prefix =3D >>>>> "/opt/pakfire/db/installed/meta- >>>>> "; >>>>> + const char *metadata_key =3D "Services"; >>>>> + const char *keyvalue_delim =3D ":"; >>>>> + const char *service_delim =3D " "; >>>>> + char *token; >>>>> + char **services =3D NULL; >>>>> + char *service; >>>>> + char *line =3D NULL; >>>>> + size_t line_len =3D 0; >>>>> + int i =3D 0; >>>>> + char *metafile; >>>>> +=20 >>>>> + if (addon =3D=3D NULL) { >>>>> + errno =3D EINVAL; >>>>> + return NULL; >>>>> + } >>>>> + >>>>> + if (asprintf(&metafile, "%s%s", metafile_prefix, addon) =3D=3D - >>>>> 1) >>>>> { >>>>> + errno =3D ENOMEM; >>>>> + return NULL; >>>>> + } >>>>=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 >>>>> + >>>>> + FILE *fp =3D fopen(metafile,"r"); >>>>> + if (fp !=3D NULL) { >>>>=20 >>>> Just FYI, I like writing this check as =E2=80=9Cif (!fp)=E2=80=9D becaus= e 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 s= till works. >>=20 >> My code won=E2=80=99t. But I am sure their code won=E2=80=99t work any mor= e 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 :) >=20 > No problem. I think you are in the right position to demand a specific > code style for use on IPFire :-) I don=E2=80=99t know what it requires to demand something very specific. I am just trying to forward my experience since I made lots of mistakes mysel= f and learned from it here and there - and there is tons of stuff that I didn= =E2=80=99t quite learn, yet. >=20 >>=20 >>>=20 >>>>=20 >>>>> + // Get initscript(s) for addon from meta-file >>>>> + while (getline(&line, &line_len, fp) !=3D -1 && services =3D=3D >>>>> NULL) { >>>>> + // Strip newline >>>>> + char *newline =3D strchr(line, '\n'); >>>>> + if (newline) *newline =3D 0; >>>>=20 >>>> Yes, this will cut off the trailing newline. >>>>=20 >>>>> + >>>>> + // Parse key/value and look for required key. >>>>> + token =3D strtok(line, keyvalue_delim); >>>>> + if (token !=3D NULL && strcmp(token, metadata_key) =3D=3D 0) >>>>> { >>>>> + token =3D strtok(NULL, keyvalue_delim); >>>>> + if (token !=3D NULL) { >>>>> + // Put each service in services array >>>>> + service =3D strtok(token, service_delim); >>>>> + while (service !=3D NULL) { >>>>> + // if filter is set, only select filtered >>>>> service >>>>> + if ((filter !=3D NULL && strcmp(filter, >>>>> service) =3D=3D 0) ||=20 >>>>> + filter =3D=3D NULL) { >>>>> + services =3D reallocarray(services ,i+1 >>>>> ,sizeof (char *)); >>>>> + if (services !=3D NULL) >>>>> + services[i++] =3D strdup(service); >>=20 >> In this block, I would find the !filter way a lot easier to read. >=20 > Agreed >=20 >>=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 >>>>> + else=20 >>>>> + break; >>>>> + } >>>>> + service =3D strtok(NULL, service_delim); >>>>> + } >>>>> + } >>>>> + } >>>>> + } >>>>> + >>>>> + 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 >>>>> + fclose(fp); >>>>> + } else { >>>>> + snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not >>>>> found.\n\n%s", addon, usage); >>>>> + } >>>>> + >>>>> + free(metafile); >>>>> + *servicescnt =3D i; >>>>> + return services; >>>>> +} >>>>> + >>>>> +// Calls initscript with parameter >>>>> +int initscript_action(const char *service, const char *action) >>>>> { >>>>> + const char *initd_path =3D "/etc/rc.d/init.d"; >>>>> + char *initscript; >>>>=20 >>>> Same here. Please initialise it. >>>>=20 >>>>> + char *argv[] =3D { >>>>> + action, >>>>> + NULL >>>>> + }; >>>>> + int r =3D 0; >>>>> + >>>>> + if ((r =3D asprintf(&initscript, "%s/%s", initd_path, >>>>> service)) >>>>> !=3D -1) { >>>>> + r =3D run(initscript, argv); >>>>> + free(initscript); >>>>> + } else { >>>>> + errno =3D ENOMEM; >>>>> + } >>>>=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 >>>>> + >>>>> + return r;=20 >>>>> +} >>>>> + >>>>> +// Move an initscript with filepattern from to >>>>> >>>>> +// Returns: >>>>> +// -1: Error during move or memory allocation. Details in >>>>> errno >>>>> +// 0: Success >>>>> +// 1: file was not moved, but is already in >>>>> +// 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) { >>>>> + char *src =3D NULL; >>>>> + char *dest =3D NULL; >>>>> + int r =3D 1; >>>>> + char *filename =3D NULL; >>>>> + >>>>> + if ((filename =3D find_file_in_dir(src_path, filepattern)) !=3D >>>>> NULL) { >>>>> + if ((r =3D asprintf(&src, "%s/%s", src_path, filename)) !=3D - >>>>> 1 && >>>>> + (r =3D asprintf(&dest, "%s/%s", dest_path, filename) !=3D >>>>> -1)) { >>>>> + // move initscript >>>>> + r =3D rename(src, dest); >>>>> + } else { >>>>> + errno =3D ENOMEM; >>>>> + } >>>>> + >>>>> + if (src !=3D NULL) >>>>> + free(src); >>>>> + if (dest !=3D NULL) >>>>> + free(dest); >>>>> + } else { >>>>> + if ((filename =3D find_file_in_dir(dest_path, filepattern)) >>>>> =3D=3D NULL)=20 >>>>> + r =3D 2; >>>>> + } >>>>> + >>>>> + if (filename !=3D 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;=20 >>>>=20 >>>> Init to NULL. >>>>=20 >>>>> + int r =3D 0; >>>>> +=20 >>>>> + if (asprintf(&filepattern, "S??%s", service) =3D=3D -1) { >>>>> + errno =3D ENOMEM; >>>>> + return -1; >>>>> + } >>>>> + >>>>> + if (strcmp(action, "enable") =3D=3D 0) { >>>>> + src_path =3D disabled_path; >>>>> + dest_path =3D enabled_path; >>>>> + } else { >>>>> + src_path =3D enabled_path; >>>>> + dest_path =3D disabled_path; >>>>> + } >>>>> + >>>>> + // Ensure disabled_path exists >>>>> + errno =3D 0; >>>>> + if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP + >>>>> S_IROTH >>>>> + S_IXOTH) =3D=3D -1 && errno !=3D EEXIST) { >>>>> + r =3D 1; >>>>> + snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s. >>>>> (Error: %d)", disabled_path, errno); >>>>> + } else { >>>>> + r =3D move_initscript_by_pattern(src_path, dest_path, >>>>> filepattern); >>>>> + if (r =3D=3D -1 ) { >>>>> + r =3D 1; >>>>> + snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s. >>>>> (Error: %d)", action, service, errno); >>>>> + } else if (r =3D=3D 1) { >>>>> + snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is >>>>> already %sd. Skipping...", service, action); >>>>> + } else if (r =3D=3D 2) { >>>>> + snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s >>>>> service %s. (Service has no valid symlink in %s).", action, >>>>> service, src_path); >>>>> + } >>>>> + } >>>>> + >>>>> + free(filepattern); >>>>> +=20 >>>>> + 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) { >>>>> + char *filepattern; >>>>=20 >>>> NULL. How does this even run without it? >>>>=20 >>>>> + if (asprintf(&filepattern, "S??%s", service) =3D=3D -1) { >>>>> + errno =3D ENOMEM; >>>>> + return; >>>>> + } >>>>> + >>>>> + if (find_file_in_dir(enabled_path, filepattern) !=3D NULL)=20 >>>>> + fprintf(stdout, "%s is enabled on boot.\n", service); >>>>> + else if (find_file_in_dir(disabled_path, filepattern) !=3D >>>>> NULL) >=20 > 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? I don=E2=80=99t quite get what you are asking for. I personally always store stuff in a variable first, and then usually evaluat= e it. One logical step in the code, per line. In that case, you will have the pointer, and you can free the space later. >=20 >>>>> + 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 =3D 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 =3D 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") =3D=3D 0) { >>>>> - snprintf(command, BUFFER_SIZE - 1, >>>>> "/etc/rc.d/init.d/%s start", name); >>>>> - safe_system(command); >>>>> - } else if (strcmp(argv[2], "stop") =3D=3D 0) { >>>>> - snprintf(command, BUFFER_SIZE - 1, >>>>> "/etc/rc.d/init.d/%s stop", name); >>>>> - safe_system(command); >>>>> - } else if (strcmp(argv[2], "restart") =3D=3D 0) { >>>>> - snprintf(command, BUFFER_SIZE - 1, >>>>> "/etc/rc.d/init.d/%s restart", name); >>>>> - safe_system(command); >>>>> - } else if (strcmp(argv[2], "reload") =3D=3D 0) { >>>>> - snprintf(command, BUFFER_SIZE - 1, >>>>> "/etc/rc.d/init.d/%s reload", name); >>>>> - safe_system(command); >>>>> - } else if (strcmp(argv[2], "status") =3D=3D 0) { >>>>> - snprintf(command, BUFFER_SIZE - 1, >>>>> "/etc/rc.d/init.d/%s status", name); >>>>> - safe_system(command); >>>>> - } else if (strcmp(argv[2], "enable") =3D=3D 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") =3D=3D 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 =3D NULL; >>>>> + int servicescnt =3D 0; >>>>> + char *addon =3D argv[1]; >>>>> + char *action =3D argv[2]; >>>>> + char *service_filter =3D NULL; >>>>> + int r =3D 0; >>>>> + >>>>> + if (!(initsetuid())) >>>>> + exit(1); >>>>> + >>>>> + if (argc < 3) { >>>>> + fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage); >>>>> + exit(1); >>>>> + } >>>>> + >>>>> + if (argc =3D=3D 4 && strcmp(action, "list-services") !=3D 0) >>>>> + service_filter =3D 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 =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. >=20 > I found this cert advisory write-up recommending always settings errno > to 0 before a library call:=20 > https://wiki.sei.cmu.edu/confluence/display/c/ERR30- > .+Take+care+when+reading+errno Hmm, that will cause new problems then=E2=80=A6 If you get an error from a function, you would want to handle that. And if du= ring that, errno gets reset, you would have to copy it first which will make = code more complicated. Maybe this is because people started to rely on errno? That would be a bad id= ea, because if you call other functions, they might use and set errno, too. > I'll do it in get_addon_services, right before the library calls that > could set errno. >=20 >>=20 >>>=20 >>>>=20 >>>>> + services =3D get_addon_services(addon, &servicescnt, >>>>> service_filter); >>>>> + if (services =3D=3D NULL || *services =3D=3D 0) { >>>>> + 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. >=20 > 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 am not sure whether that advise is that practical. I understand the intention, but I believe it makes more complex code even mor= e complex. >=20 >>=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? >=20 > That varies :-)=20 > Most common suggestion is the non-helpful "use another mechanism to > report errors" >=20 > 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. Then you will make the folks angry who hate enums because they might change i= n size and what not... > Some even suggest implementing a simulation of the C standard errno > mechanism: > /* your_errno.c */ > __thread int g_your_error_code; Ah, and of course threads :) > /* 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) >=20 > 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+th= at+return+errno+with+a+return+type+of+errno_t >=20 > 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. >=20 > (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.) I do not know many libraries which follow this. Maybe because libraries are b= y definition something that is close to the core system? For many functions, it is very convenient to just return a pointer (or to ret= urn NULL). For example strdup(). It would get really complicated if strdup() = would return an integer (or enum stuff) and the duplicated string would be ha= nded over by another pointer. Maybe this is the reason why people hate C? I am not aware that other languag= es are doing it that much differently. >>=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. >=20 > Ok. I was under the impression that the document spoke about glibc but > as I understand it now, it is about gnulib :-) >=20 > I should be able to produce a new version of this patchset by Tuesday > evening. Cool! -Michael >=20 > Regards > Robin >>=20 >>>=20 >>>>=20 >>>>> + fprintf(stderr, "\nSystem error occured. (Error: >>>>> %d)\n\n", errno); >>>>> + else if (strcmp(errormsg, "") !=3D 0) >>>>> + fprintf(stderr, "\n%s\n\n", errormsg); >>>>> + else if (service_filter !=3D 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") =3D=3D 0 || >>>>> + strcmp(action, "stop") =3D=3D 0 || >>>>> + strcmp(action, "restart") =3D=3D 0 || >>>>> + strcmp(action, "reload") =3D=3D 0 || >>>>> + strcmp(action, "status") =3D=3D 0) { >>>>> + >>>>> + errno =3D 0; >>>>> + for(int i =3D 0; i < servicescnt; i++) { >>>>> + if (initscript_action(services[i], action) !=3D 0) { >>>>> + r =3D 1; >>>>> + if (errno !=3D 0)=20 >>>>> + fprintf(stderr, "\nSystem error occured. >>>>> (Error: %d)\n\n", errno); >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + } else if (strcmp(action, "enable") =3D=3D 0 || >>>>> + strcmp(action, "disable") =3D=3D 0) { >>>>> + >>>>> + errno =3D 0; >>>>> + for(int i =3D 0; i < servicescnt; i++) { >>>>> + if (toggle_service(services[i], action) =3D=3D 0) { >>>>> + fprintf(stdout, "%sd service %s\n", action, >>>>> services[i]); >>>>=20 >>>> If you want to just print something, just use printf. >>>=20 >>> Ok >>>=20 >>>>=20 >>>>> + } else if (errno !=3D 0) { >>>>> + r =3D 1; >>>>> + fprintf(stderr, "\nSystem error occured. (Error: >>>>> %d)\n\n", errno); >>>>> + break; >>>>> + } else { >>>>> + r =3D 1; >>>>> + fprintf(stderr, "\n%s\n\n", errormsg); >>>>> + } >>>>> + } >>>>> + >>>>> + } else if (strcmp(action, "boot-status") =3D=3D 0) { >>>>> + errno =3D 0; >>>>> + for(int i =3D 0; i < servicescnt; i++) { >>>>> + print_boot_status(services[i]); >>>>> + if (errno !=3D 0) { >>>>> + r =3D 1; >>>>> + fprintf(stderr, "\nSystem error occured. (Error: >>>>> %d)\n\n", errno); >>>>> + break; >>>>> + } >>>>> + } >>>>> +=20 >>>>> + } else if (strcmp(action, "list-services") =3D=3D 0) { >>>>> + fprintf(stdout, "\nServices for addon %s:\n", addon); >>>>> + for(int i =3D 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 =3D 1; >>>>> + } >>>>> + >>>>> + // Cleanup >>>>> + for(int i =3D 0; i < servicescnt; i++)=20 >>>>> + free(services[i]); >>>>> + free(services); >>>>> + >>>>> + 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 > --=20 > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn. --===============5193840404465553484==--