* [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements
@ 2022-10-06 17:59 Robin Roevens
2022-10-06 17:59 ` [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata Robin Roevens
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Robin Roevens @ 2022-10-06 17:59 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]
Hi all
After processing, tackling and implementing all remarks Michael gave
about previous version, I hereby present you v2 of this patchset.
As the other patchset that I submitted, containing small cosmetic
changes on services.cgi, depended on v1 of this patchset; I have now
included those changes in this set instead of reposting those again as a
seperate set. Those cosmetics (patch 3-5) are not part of the fix for
bug#12935.
I'm quite confident that the code of addonctrl is now much
better/cleaner by implementing and handling Michael's concerns. There
is now even more errorchecking going on (mainly for system errors).
I know Core 171 went in Testing, but as there currently are already 2
bugreports concering this fix (#12916 was also linked to this today);
I'm assuming quite a few IPFire users are currently experiencing
problems with the services.cgi page. So, if possible and feasable, I
think it would be a good idea to try to include this one in CU171 (at
least patch 1 and 2) ?
If there are still concerns/remarks about this new version, I'll try to
handle them asap.
For reference, the content of the summary mail that was sent with v1 of
the patch:
---
This patchset fixes Bug#12935
(https://bugzilla.ipfire.org/show_bug.cgi?id=12935)
Summary:
Addons where the initscript does not match the addon-name and addons with
multiple initscripts are now listed on services.cgi since CU170.
But addonctrl still expected addon name to be equal to
initscript name; Hence starting/stopping/enabling/disabling of such
addons was not possible.
This has always been like that, but that problem was hidden as
services.cgi also did not display those addon services.
After discussing this with Adolf on the Bug report, we concluded that we
should adapt addonctrl to work with the new addon metadata
Services-field instead.
I basically rewrote addonctrl to not only use the new services metadata
but also to have better errorchecking and added the posibility to check
if a service is currently enabled or disabled.
As a result services.cgi no longer has to go checking the precense of
runlevel initscripts, but can just ask addonctrl.
I also added a warning to services.cgi if a runlevel initscript does not
exists, to prevent the user from wondering why he can't enable a
specific service. (Adolf pointed out some services don't install
runlevel initscripts by default)
More details in the bugreport and in the commit-messages of the patches.
---
Regards
Robin
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata
2022-10-06 17:59 [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements Robin Roevens
@ 2022-10-06 17:59 ` Robin Roevens
2022-10-07 15:01 ` Michael Tremer
2022-10-06 17:59 ` [PATCH v2 2/5] services.cgi: Fix status/actions on services with name != addon name Robin Roevens
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Robin Roevens @ 2022-10-06 17:59 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 15468 bytes --]
* 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;
+ }
+
+ FILE *fp = fopen(metafile,"r");
+ if (fp != NULL) {
+ // 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;
+
+ // 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);
+ else
+ break;
+ }
+ service = strtok(NULL, service_delim);
+ }
+ }
+ }
+ }
+
+ free(line);
+ 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;
+ 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;
+ }
+
+ 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;
+ 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;
+ 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)
+ 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;
+ services = get_addon_services(addon, &servicescnt, service_filter);
+ if (services == NULL || *services == 0) {
+ if (errno != 0)
+ 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]);
+ } 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;
}
--
2.37.3
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/5] services.cgi: Fix status/actions on services with name != addon name
2022-10-06 17:59 [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements Robin Roevens
2022-10-06 17:59 ` [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata Robin Roevens
@ 2022-10-06 17:59 ` Robin Roevens
2022-10-06 17:59 ` [PATCH v2 3/5] services.cgi: minor cosmetics Robin Roevens
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Robin Roevens @ 2022-10-06 17:59 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 9983 bytes --]
* addonctrl's new functionality to control explicit addon services was
implemented.
* Change 'Addon' column header to 'Addon Service' to be clear that
it's not addons but services listed here.
* Services not matching the name of the addon now display the addon
name between parentheses, so the user knows where the service comes
from.
* When no valid runlevel symlink is found by addonctrl for a service,
the 'enable on boot' checkbox is replaced by a small exclamation point
with alt-text "No valid runlevel symlink was found for the initscript of
this service." to inform user why a service can't be enabled.
* Added German and Dutch translation for above message.
Fixes: Bug#12935
Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
---
html/cgi-bin/services.cgi | 114 ++++++++++++++++----------------------
langs/de/cgi-bin/de.pl | 1 +
langs/en/cgi-bin/en.pl | 1 +
langs/nl/cgi-bin/nl.pl | 1 +
4 files changed, 51 insertions(+), 66 deletions(-)
diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
index 29926ecc3..de946d755 100644
--- a/html/cgi-bin/services.cgi
+++ b/html/cgi-bin/services.cgi
@@ -20,7 +20,7 @@
###############################################################################
use strict;
-
+use feature "switch";
# enable only the following on debugging purpose
#use warnings;
#use CGI::Carp 'fatalsToBrowser';
@@ -141,15 +141,22 @@ END
&Header::openbox('100%', 'left', "Addon - $Lang::tr{services}");
my $paramstr=$ENV{QUERY_STRING};
my @param=split(/!/, $paramstr);
- if ($param[1] ne ''){
- &General::system("/usr/local/bin/addonctrl", "$param[0]", "$param[1]");
+ # Make sure action parameter is actually one of the allowed service actions
+ given ($param[1]) {
+ when ( ['start', 'stop', 'enable', 'disable'] ) {
+ # Make sure pak-name and service name don't contain any illegal character
+ if ( $param[0] !~ /[^a-zA-Z_0-9\-]/ &&
+ $param[2] !~ /[^a-zA-Z_0-9\-]/ ) {
+ &General::system("/usr/local/bin/addonctrl", "$param[0]", "$param[1]", "$param[2]");
+ }
+ }
}
print <<END
<div align='center'>
<table width='80%' cellspacing='1' class='tbl'>
<tr>
- <th align='center'><b>Addon</b></th>
+ <th align='left'><b>Addon $Lang::tr{service}</b></th>
<th align='center'><b>Boot</b></th>
<th align='center' colspan=2><b>$Lang::tr{'action'}</b></th>
<th align='center'><b>$Lang::tr{'status'}</b></th>
@@ -170,33 +177,35 @@ END
foreach my $pak (keys %paklist) {
my %metadata = &Pakfire::getmetadata($pak, "installed");
+ my $service;
+
if ("$metadata{'Services'}") {
- foreach my $service (split(/ /, "$metadata{'Services'}")) {
- push(@addon_services, $service);
- }
- }
- }
+ foreach $service (split(/ /, "$metadata{'Services'}")) {
+ $lines++;
+ if ($lines % 2) {
+ print "<tr>";
+ $col="bgcolor='$color{'color22'}'";
+ } else {
+ print "<tr>";
+ $col="bgcolor='$color{'color20'}'";
+ }
- foreach (@addon_services) {
- $lines++;
- if ($lines % 2){
- print "<tr>";
- $col="bgcolor='$color{'color22'}'";
- }else{
- print "<tr>";
- $col="bgcolor='$color{'color20'}'";
+ # Add addon name to displayname of service if servicename differs from addon
+ my $displayname = ($pak ne $service) ? "$service ($pak)" : $service;
+ print "<td align='left' $col width='31%'>$displayname</td> ";
+
+ my $status = isautorun($pak,$service,$col);
+ print "$status ";
+ print "<td align='center' $col width='8%'><a href='services.cgi?$pak!start!$service'><img alt='$Lang::tr{'start'}' title='$Lang::tr{'start'}' src='/images/go-up.png' border='0' /></a></td>";
+ print "<td align='center' $col width='8%'><a href='services.cgi?$pak!stop!$service'><img alt='$Lang::tr{'stop'}' title='$Lang::tr{'stop'}' src='/images/go-down.png' border='0' /></a></td> ";
+ my $status = isrunningaddon($pak,$service,$col);
+ $status =~ s/\^[\[[0-1]\;[0-9]+m//g;
+
+ chomp($status);
+ print "$status";
+ print "</tr>";
+ }
}
- print "<td align='left' $col width='31%'>$_</td> ";
- my $status = isautorun($_,$col);
- print "$status ";
- print "<td align='center' $col width='8%'><a href='services.cgi?$_!start'><img alt='$Lang::tr{'start'}' title='$Lang::tr{'start'}' src='/images/go-up.png' border='0' /></a></td>";
- print "<td align='center' $col width='8%'><a href='services.cgi?$_!stop'><img alt='$Lang::tr{'stop'}' title='$Lang::tr{'stop'}' src='/images/go-down.png' border='0' /></a></td> ";
- my $status = isrunningaddon($_,$col);
- $status =~ s/\^[\[[0-1]\;[0-9]+m//g;
-
- chomp($status);
- print "$status";
- print "</tr>";
}
print "</table></div>\n";
@@ -215,51 +224,24 @@ END
}
sub isautorun (@) {
- my ($cmd, $col) = @_;
-
- # Init directory.
- my $initdir = "/etc/rc.d/rc3.d/";
-
- my $status = "<td align='center' $col></td>";
+ my ($pak, $service, $col) = @_;
+ my @testcmd = &General::system_output("/usr/local/bin/addonctrl", "$pak", "boot-status", "$service");
+ my $testcmd = @testcmd[0];
+ my $status = "<td align='center' $col><img alt='$Lang::tr{'service boot setting unavailable'}' title='$Lang::tr{'service boot setting unavailable'}' src='/images/dialog-warning.png' border='0' width='16' height='16' /></td>";
- # Check if autorun for the given cmd is enabled.
- if ( &find_init("$cmd", "$initdir") ) {
+ # Check if autorun for the given service is enabled.
+ if ( $testcmd =~ /enabled\ on\ boot/ ) {
# Adjust status.
- $status = "<td align='center' $col><a href='services.cgi?$_!disable'><img alt='$Lang::tr{'deactivate'}' title='$Lang::tr{'deactivate'}' src='/images/on.gif' border='0' width='16' height='16' /></a></td>";
- } else {
+ $status = "<td align='center' $col><a href='services.cgi?$pak!disable!$service'><img alt='$Lang::tr{'deactivate'}' title='$Lang::tr{'deactivate'}' src='/images/on.gif' border='0' width='16' height='16' /></a></td>";
+ } elsif ( $testcmd =~ /disabled\ on\ boot/ ) {
# Adjust status.
- $status = "<td align='center' $col><a href='services.cgi?$_!enable'><img alt='$Lang::tr{'activate'}' title='$Lang::tr{'activate'}' src='/images/off.gif' border='0' width='16' height='16' /></a></td>";
+ $status = "<td align='center' $col><a href='services.cgi?$pak!enable!$service'><img alt='$Lang::tr{'activate'}' title='$Lang::tr{'activate'}' src='/images/off.gif' border='0' width='16' height='16' /></a></td>";
}
# Return the status.
return $status;
}
-sub find_init (@) {
- my ($cmd, $dir) = @_;
-
- # Open given init directory.
- opendir (INITDIR, "$dir") || die "Cannot opendir $dir: $!";
-
- # Read-in init files from directory.
- my @inits = readdir(INITDIR);
-
- # Close directory handle.
- closedir(INITDIR);
-
- # Loop through the directory.
- foreach my $init (@inits) {
- # Check if the current processed file belongs to the given command.
- if ($init =~ /S\d+\d+$cmd\z/) {
- # Found, return "1" - True.
- return "1";
- }
- }
-
- # Nothing found, return nothing.
- return;
-}
-
sub isrunning (@) {
my ($cmd, $col) = @_;
my $status = "<td align='center' bgcolor='${Header::colourred}'><font color='white'><b>$Lang::tr{'stopped'}</b></font></td><td colspan='2' $col></td>";
@@ -313,7 +295,7 @@ sub isrunning (@) {
}
sub isrunningaddon (@) {
- my ($cmd, $col) = @_;
+ my ($pak, $service, $col) = @_;
my $status = "<td align='center' bgcolor='${Header::colourred}'><font color='white'><b>$Lang::tr{'stopped'}</b></font></td><td colspan='2' $col></td>";
my $pid = '';
@@ -321,7 +303,7 @@ sub isrunningaddon (@) {
my $exename;
my @memory;
- my @testcmd = &General::system_output("/usr/local/bin/addonctrl", "$cmd", "status");
+ my @testcmd = &General::system_output("/usr/local/bin/addonctrl", "$pak", "status", "$service");
my $testcmd = @testcmd[0];
if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\ running/){
diff --git a/langs/de/cgi-bin/de.pl b/langs/de/cgi-bin/de.pl
index 798abcffc..db7d117b0 100644
--- a/langs/de/cgi-bin/de.pl
+++ b/langs/de/cgi-bin/de.pl
@@ -2251,6 +2251,7 @@
'server string' => 'Server String',
'service' => 'Dienst',
'service added' => 'Benutzerdefinierter Netzwerkdienst wurde hinzugefügt',
+'service boot setting unavailable' => 'Für das Initscript dieses Dienstes wurde kein gültiger Runlevel-Symlink gefunden.',
'service name' => 'Name des Dienstes:',
'service removed' => 'Benutzerdefinierter Netzwerkdienst wurde entfernt',
'service updated' => 'Benutzerdefinierter Netzwerkdienst wurde aktualisiert',
diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl
index f770e7cd9..60dca5be4 100644
--- a/langs/en/cgi-bin/en.pl
+++ b/langs/en/cgi-bin/en.pl
@@ -2306,6 +2306,7 @@
'server string' => 'Server String',
'service' => 'Service',
'service added' => 'Custom network service added',
+'service boot setting unavailable' => 'No valid runlevel symlink was found for the initscript of this service.',
'service name' => 'Service name:',
'service removed' => 'Custom network service removed',
'service updated' => 'Custom network service updated',
diff --git a/langs/nl/cgi-bin/nl.pl b/langs/nl/cgi-bin/nl.pl
index 49dabec99..4fd6955cc 100644
--- a/langs/nl/cgi-bin/nl.pl
+++ b/langs/nl/cgi-bin/nl.pl
@@ -1899,6 +1899,7 @@
'server string' => 'Server String',
'service' => 'Dienst',
'service added' => 'Aangepaste netwerkdienst toegevoegd',
+'service boot setting unavailable' => 'Er werd voor het initscript van deze service geen geldige runlevel symlink gevonden.',
'service name' => 'Dienstennaam:',
'service removed' => 'Aangepaste netwerkdienst verwijderd',
'service updated' => 'Aangepaste netwerkdienst bijgewerkt',
--
2.37.3
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/5] services.cgi: minor cosmetics
2022-10-06 17:59 [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements Robin Roevens
2022-10-06 17:59 ` [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata Robin Roevens
2022-10-06 17:59 ` [PATCH v2 2/5] services.cgi: Fix status/actions on services with name != addon name Robin Roevens
@ 2022-10-06 17:59 ` Robin Roevens
2022-10-06 18:52 ` Bernhard Bitsch
2022-10-06 17:59 ` [PATCH v2 4/5] services.cgi: add restart action and restrict action usage Robin Roevens
2022-10-06 17:59 ` [PATCH v2 5/5] services.cgi: add link to addon config if ui exists for it Robin Roevens
4 siblings, 1 reply; 13+ messages in thread
From: Robin Roevens @ 2022-10-06 17:59 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]
* Singular 'Service' instead of plural 'Services' as column header of
services table
* Sort list of services
Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
---
html/cgi-bin/services.cgi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
index de946d755..e35b04cae 100644
--- a/html/cgi-bin/services.cgi
+++ b/html/cgi-bin/services.cgi
@@ -105,7 +105,7 @@ if ( $querry[0] =~ "processescpu"){
<div align='center'>
<table width='80%' cellspacing='1' class='tbl'>
<tr>
- <th align='left'><b>$Lang::tr{'services'}</b></th>
+ <th align='left'><b>$Lang::tr{'service'}</b></th>
<th align='center' ><b>$Lang::tr{'status'}</b></th>
<th align='center'><b>PID</b></th>
<th align='center'><b>$Lang::tr{'memory'}</b></th>
@@ -174,7 +174,7 @@ END
# Generate list of installed addon pak services
my %paklist = &Pakfire::dblist("installed");
- foreach my $pak (keys %paklist) {
+ foreach my $pak (sort keys %paklist) {
my %metadata = &Pakfire::getmetadata($pak, "installed");
my $service;
--
2.37.3
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/5] services.cgi: add restart action and restrict action usage
2022-10-06 17:59 [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements Robin Roevens
` (2 preceding siblings ...)
2022-10-06 17:59 ` [PATCH v2 3/5] services.cgi: minor cosmetics Robin Roevens
@ 2022-10-06 17:59 ` Robin Roevens
2022-10-06 17:59 ` [PATCH v2 5/5] services.cgi: add link to addon config if ui exists for it Robin Roevens
4 siblings, 0 replies; 13+ messages in thread
From: Robin Roevens @ 2022-10-06 17:59 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]
* Add restart action to services.
* Only display available actions for a service:
Start when service is stopped or Stop and Restart when a service
is running.
Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
---
html/cgi-bin/services.cgi | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
index e35b04cae..4b379251e 100644
--- a/html/cgi-bin/services.cgi
+++ b/html/cgi-bin/services.cgi
@@ -143,7 +143,7 @@ END
my @param=split(/!/, $paramstr);
# Make sure action parameter is actually one of the allowed service actions
given ($param[1]) {
- when ( ['start', 'stop', 'enable', 'disable'] ) {
+ when ( ['start', 'stop', 'restart', 'enable', 'disable'] ) {
# Make sure pak-name and service name don't contain any illegal character
if ( $param[0] !~ /[^a-zA-Z_0-9\-]/ &&
$param[2] !~ /[^a-zA-Z_0-9\-]/ ) {
@@ -196,8 +196,6 @@ END
my $status = isautorun($pak,$service,$col);
print "$status ";
- print "<td align='center' $col width='8%'><a href='services.cgi?$pak!start!$service'><img alt='$Lang::tr{'start'}' title='$Lang::tr{'start'}' src='/images/go-up.png' border='0' /></a></td>";
- print "<td align='center' $col width='8%'><a href='services.cgi?$pak!stop!$service'><img alt='$Lang::tr{'stop'}' title='$Lang::tr{'stop'}' src='/images/go-down.png' border='0' /></a></td> ";
my $status = isrunningaddon($pak,$service,$col);
$status =~ s/\^[\[[0-1]\;[0-9]+m//g;
@@ -307,7 +305,9 @@ sub isrunningaddon (@) {
my $testcmd = @testcmd[0];
if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\ running/){
- $status = "<td align='center' bgcolor='${Header::colourgreen}'><font color='white'><b>$Lang::tr{'running'}</b></font></td>";
+ $status = "<td align='center' $col width='8%'><a href='services.cgi?$pak!stop!$service'><img alt='$Lang::tr{'stop'}' title='$Lang::tr{'stop'}' src='/images/go-down.png' border='0' /></a></td> ";
+ $status .= "<td align='center' $col width='8%'><a href='services.cgi?$pak!restart!$service'><img alt='$Lang::tr{'restart'}' title='$Lang::tr{'restart'}' src='/images/reload.gif' border='0' /></a></td> ";
+ $status .= "<td align='center' bgcolor='${Header::colourgreen}'><font color='white'><b>$Lang::tr{'running'}</b></font></td>";
$testcmd =~ s/.* //gi;
$testcmd =~ s/[a-z_]//gi;
$testcmd =~ s/\[[0-1]\;[0-9]+//gi;
@@ -330,7 +330,8 @@ sub isrunningaddon (@) {
}
$status .="<td align='center' $col>$memory KB</td>";
}else{
- $status = "<td align='center' bgcolor='${Header::colourred}'><font color='white'><b>$Lang::tr{'stopped'}</b></font></td><td colspan='2' $col></td>";
+ $status = "<td align='center' $col width='16%' colspan=2><a href='services.cgi?$pak!start!$service'><img alt='$Lang::tr{'start'}' title='$Lang::tr{'start'}' src='/images/go-up.png' border='0' /></a></td>";
+ $status .= "<td align='center' bgcolor='${Header::colourred}'><font color='white'><b>$Lang::tr{'stopped'}</b></font></td><td colspan='2' $col></td>";
}
return $status;
}
--
2.37.3
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 5/5] services.cgi: add link to addon config if ui exists for it
2022-10-06 17:59 [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements Robin Roevens
` (3 preceding siblings ...)
2022-10-06 17:59 ` [PATCH v2 4/5] services.cgi: add restart action and restrict action usage Robin Roevens
@ 2022-10-06 17:59 ` Robin Roevens
2022-10-06 18:48 ` Bernhard Bitsch
4 siblings, 1 reply; 13+ messages in thread
From: Robin Roevens @ 2022-10-06 17:59 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]
* If a cgi file exists with the same name as an addon, the
displayed service will be a link to that cgi file.
Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
---
html/cgi-bin/services.cgi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
index 4b379251e..14ed01c50 100644
--- a/html/cgi-bin/services.cgi
+++ b/html/cgi-bin/services.cgi
@@ -192,6 +192,10 @@ END
# Add addon name to displayname of service if servicename differs from addon
my $displayname = ($pak ne $service) ? "$service ($pak)" : $service;
+ if ( -e "/srv/web/ipfire/cgi-bin/$pak.cgi" ) {
+ $displayname = ($pak ne $service) ? "$service (<a href=\'$pak.cgi\'>$pak</a>)" : "<a href=\'$pak.cgi\'>$service</a>";
+ }
+
print "<td align='left' $col width='31%'>$displayname</td> ";
my $status = isautorun($pak,$service,$col);
--
2.37.3
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 5/5] services.cgi: add link to addon config if ui exists for it
2022-10-06 17:59 ` [PATCH v2 5/5] services.cgi: add link to addon config if ui exists for it Robin Roevens
@ 2022-10-06 18:48 ` Bernhard Bitsch
0 siblings, 0 replies; 13+ messages in thread
From: Bernhard Bitsch @ 2022-10-06 18:48 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
Reviewed-by: Bernhard Bitsch <bbitsch(a)ipfire.org>
Am 06.10.2022 um 19:59 schrieb Robin Roevens:
> * If a cgi file exists with the same name as an addon, the
> displayed service will be a link to that cgi file.
>
> Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> ---
> html/cgi-bin/services.cgi | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> index 4b379251e..14ed01c50 100644
> --- a/html/cgi-bin/services.cgi
> +++ b/html/cgi-bin/services.cgi
> @@ -192,6 +192,10 @@ END
>
> # Add addon name to displayname of service if servicename differs from addon
> my $displayname = ($pak ne $service) ? "$service ($pak)" : $service;
> + if ( -e "/srv/web/ipfire/cgi-bin/$pak.cgi" ) {
> + $displayname = ($pak ne $service) ? "$service (<a href=\'$pak.cgi\'>$pak</a>)" : "<a href=\'$pak.cgi\'>$service</a>";
> + }
> +
> print "<td align='left' $col width='31%'>$displayname</td> ";
>
> my $status = isautorun($pak,$service,$col);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/5] services.cgi: minor cosmetics
2022-10-06 17:59 ` [PATCH v2 3/5] services.cgi: minor cosmetics Robin Roevens
@ 2022-10-06 18:52 ` Bernhard Bitsch
0 siblings, 0 replies; 13+ messages in thread
From: Bernhard Bitsch @ 2022-10-06 18:52 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
Reviewed-by: Bernhard Bitsch <bbitsch(a)ipfire.org>
Am 06.10.2022 um 19:59 schrieb Robin Roevens:
> * Singular 'Service' instead of plural 'Services' as column header of
> services table
> * Sort list of services
>
> Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> ---
> html/cgi-bin/services.cgi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> index de946d755..e35b04cae 100644
> --- a/html/cgi-bin/services.cgi
> +++ b/html/cgi-bin/services.cgi
> @@ -105,7 +105,7 @@ if ( $querry[0] =~ "processescpu"){
> <div align='center'>
> <table width='80%' cellspacing='1' class='tbl'>
> <tr>
> - <th align='left'><b>$Lang::tr{'services'}</b></th>
> + <th align='left'><b>$Lang::tr{'service'}</b></th>
> <th align='center' ><b>$Lang::tr{'status'}</b></th>
> <th align='center'><b>PID</b></th>
> <th align='center'><b>$Lang::tr{'memory'}</b></th>
> @@ -174,7 +174,7 @@ END
> # Generate list of installed addon pak services
> my %paklist = &Pakfire::dblist("installed");
>
> - foreach my $pak (keys %paklist) {
> + foreach my $pak (sort keys %paklist) {
> my %metadata = &Pakfire::getmetadata($pak, "installed");
>
> my $service;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata
2022-10-06 17:59 ` [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata Robin Roevens
@ 2022-10-07 15:01 ` Michael Tremer
2022-10-08 23:09 ` Robin Roevens
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tremer @ 2022-10-07 15:01 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 18528 bytes --]
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().
asprintf() will automatically set errno, actually.
> +
> + 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...
> + // 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);
Technically you could check whether strdup() was successful, but I would accept this version.
> + 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?
> + 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.
> +
> + 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)
> + 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.
> + 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
}
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.
> + 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.
> + } 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 :)
-Michael
> --
> 2.37.3
>
>
> --
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata
2022-10-07 15:01 ` Michael Tremer
@ 2022-10-08 23:09 ` Robin Roevens
2022-10-10 10:04 ` Michael Tremer
0 siblings, 1 reply; 13+ messages in thread
From: Robin Roevens @ 2022-10-08 23:09 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 26273 bytes --]
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.
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.
>
> 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.
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.
But if you think that is unnecessary I'll change it.
Should I change all my other checks against NULL also? Or only the
filepointer ?
>
> > + // 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);
>
> 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.
>
> > + 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)
> > + 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.
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.
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.
>
> > + 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?
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. 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
>
> > + 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.
Robin
>
> -Michael
>
> > --
> > 2.37.3
> >
> >
> > --
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> >
>
>
--
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata
2022-10-08 23:09 ` Robin Roevens
@ 2022-10-10 10:04 ` Michael Tremer
2022-10-10 12:27 ` Robin Roevens
0 siblings, 1 reply; 13+ messages in thread
From: Michael Tremer @ 2022-10-10 10:04 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 24555 bytes --]
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 :)
>
>>
>>> + // 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.
>>
>> 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)
>>> + 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.
>
>>
>>> + 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.
> 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?
> 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.
>
>>
>>> + 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
>>
>>> --
>>> 2.37.3
>>>
>>>
>>> --
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>>
>>
>>
>
> --
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata
2022-10-10 10:04 ` Michael Tremer
@ 2022-10-10 12:27 ` Robin Roevens
2022-10-10 13:27 ` Michael Tremer
0 siblings, 1 reply; 13+ messages in thread
From: Robin Roevens @ 2022-10-10 12:27 UTC (permalink / raw)
To: development
[-- 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata
2022-10-10 12:27 ` Robin Roevens
@ 2022-10-10 13:27 ` Michael Tremer
0 siblings, 0 replies; 13+ messages in thread
From: Michael Tremer @ 2022-10-10 13:27 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 30901 bytes --]
Hello Robin,
> On 10 Oct 2022, at 13:27, Robin Roevens <robin.roevens(a)disroot.org> wrote:
>
> 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 :-)
I don’t know what it requires to demand something very specific.
I am just trying to forward my experience since I made lots of mistakes myself and learned from it here and there - and there is tons of stuff that I didn’t quite learn, yet.
>
>>
>>>
>>>>
>>>>> + // 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?
I don’t quite get what you are asking for.
I personally always store stuff in a variable first, and then usually evaluate it. One logical step in the code, per line.
In that case, you will have the pointer, and you can free the space later.
>
>>>>> + 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
Hmm, that will cause new problems then…
If you get an error from a function, you would want to handle that. And if during 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 idea, 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.
>
>>
>>>
>>>>
>>>>> + 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 am not sure whether that advise is that practical.
I understand the intention, but I believe it makes more complex code even more complex.
>
>>
>>> 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.
Then you will make the folks angry who hate enums because they might change in 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 = (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.)
I do not know many libraries which follow this. Maybe because libraries are by definition something that is close to the core system?
For many functions, it is very convenient to just return a pointer (or to return NULL). For example strdup(). It would get really complicated if strdup() would return an integer (or enum stuff) and the duplicated string would be handed over by another pointer.
Maybe this is the reason why people hate C? I am not aware that other languages are doing it that much differently.
>>
>>> 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.
Cool!
-Michael
>
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-10-10 13:27 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 17:59 [PATCH v2 0/5] Fix Bug#12935 + cosmetic changes/enhancements Robin Roevens
2022-10-06 17:59 ` [PATCH v2 1/5] misc-progs: addonctrl: Add support for 'Services' metadata Robin Roevens
2022-10-07 15:01 ` Michael Tremer
2022-10-08 23:09 ` Robin Roevens
2022-10-10 10:04 ` Michael Tremer
2022-10-10 12:27 ` Robin Roevens
2022-10-10 13:27 ` Michael Tremer
2022-10-06 17:59 ` [PATCH v2 2/5] services.cgi: Fix status/actions on services with name != addon name Robin Roevens
2022-10-06 17:59 ` [PATCH v2 3/5] services.cgi: minor cosmetics Robin Roevens
2022-10-06 18:52 ` Bernhard Bitsch
2022-10-06 17:59 ` [PATCH v2 4/5] services.cgi: add restart action and restrict action usage Robin Roevens
2022-10-06 17:59 ` [PATCH v2 5/5] services.cgi: add link to addon config if ui exists for it Robin Roevens
2022-10-06 18:48 ` Bernhard Bitsch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox