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: Fri, 07 Oct 2022 16:01:58 +0100 Message-ID: In-Reply-To: <20221006175958.11036-2-robin.roevens@disroot.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5536880312140666515==" List-Id: --===============5536880312140666515== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello again, > 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 s= ervices. 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; > + } You should initialise metafile with NULL before passing it to asprintf(). asprintf() will automatically set errno, actually. > + > + FILE *fp =3D fopen(metafile,"r"); > + if (fp !=3D NULL) { Just FYI, I like writing this check as =E2=80=9Cif (!fp)=E2=80=9D because it = is a a lot shorter... > + // Get initscript(s) for addon from meta-file > + while (getline(&line, &line_len, fp) !=3D -1 && services =3D=3D NU= LL) { > + // Strip newline > + char *newline =3D strchr(line, '\n'); > + if (newline) *newline =3D 0; Yes, this will cut off the trailing newline. > + > + // 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 ,sizeo= f (char *)); > + if (services !=3D NULL) > + services[i++] =3D strdup(service); Technically you could check whether strdup() was successful, but I would acce= pt this version. > + else=20 > + break; > + } > + service =3D 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 witho= ut ever allocating a buffer? > + 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; Same here. Please initialise it. > + 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; > + } This could look slightly cleaner as: =E2=80=A6 r =3D asprintf(&initscript, =E2=80=A6.); if (r > 0) { r =3D run(initscript, argv); } if (initscript) free(initscript); return r; Maybe=E2=80=A6 I don=E2=80=99t know. > + > + 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 di= sabled_path > +int toggle_service(const char *service, const char *action) { > + const char *src_path, *dest_path; > + char *filepattern;=20 Init to NULL. > + 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_IXO= TH) =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; NULL. How does this even run without it? > + 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 > + fprintf(stdout, "%s is disabled on boot.\n", service); > + else > + fprintf(stdout, "%s is not available for boot. (Service has no val= id 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|res= tart|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|re= start|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|st= op|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|re= start|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; There is usually no need to reset this. > + services =3D get_addon_services(addon, &servicescnt, service_filter); > + if (services =3D=3D NULL || *services =3D=3D 0) { > + if (errno !=3D 0) This is not reliable error checking. The function should return a defined val= ue if an error has occurred (e.g. NULL) and an error is detected, that error = should be handled. So in this case: services =3D get_addon_services(=E2=80=A6); if (!services) { printf(=E2=80=9CERROR: =E2=80=A6\=E2=80=9D); } =E2=80=A6 Other code, on success=E2=80=A6 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 act= ually 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 =3D get_addon_services(=E2=80=A6); if (!services) { switch (errno) { case ENOENT: DO SOMETHING; Break; default: printf(=E2=80=9CERROR: =E2=80=A6\=E2=80=9D); DO SOMETING ELSE } Finally, instead of printing the errno number in the error message, you can u= se %m (without any argument) which will be automatically converted into somet= hing human readable. > + fprintf(stderr, "\nSystem error occured. (Error: %d)\n\n", err= no); > + 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]); If you want to just print something, just use printf. > + } 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; > } Otherwise, this looks a lot better since the last version :) -Michael > --=20 > 2.37.3 >=20 >=20 > --=20 > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn. >=20 --===============5536880312140666515==--