On 26.03.2024 06:15, zithro / Cyril Rébert wrote:
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -8,26 +8,34 @@
> #include <stdlib.h>
> #include <unistd.h>
> #include <signal.h>
> +#include <string.h>
> #include <stdio.h>
>
> +#define _GNU_SOURCE
This wants defining first thing in a file (or even on the compiler command
line), to (properly) affect all headers.
> +#include <getopt.h>
> +#if !defined(__GNUC__) && !defined(__GNUG__)
Why __GNUG__ in a C file? And anyway, why is this construct needed? You
don't ...
> +#define __attribute__(arg) /* empty */
... use any attributes down from here.
You mention xentop as where you've taken them from. I view those as
questionable. If in fact you had used any other utility's source for
reference, you wouldn't have encountered such.
> +#endif
> +
> xc_interface *h;
> int id = 0;
>
> void daemonize(void)
> {
> switch (fork()) {
> - case -1:
> - err(1, "fork");
> - case 0:
> - break;
> - default:
> - exit(0);
> + case -1:
> + err(1, "fork");
> + case 0:
> + break;
> + default:
> + exit(0);
> }
The case labels were properly indented, weren't they? And all other re-
indentation you do wants splitting from the functional change.
> @@ -52,39 +60,68 @@ void catch_usr1(int sig)
>
> int main(int argc, char **argv)
> {
> +
> int t, s;
What would this new blank line be good for?
> int ret;
> +
I'd even question this one.
> + char *usage = "usage: %s <timeout> <sleep>";
static const char[]
> + int opt, optind = 0;
> +
> + struct option lopts[] = {
static const
> + { "help", no_argument, NULL, 'h' },
> + };
> + const char *sopts = "h";
> +
> + while ((opt = getopt_long(argc, argv, sopts, lopts, &optind)) != -1) {
> + switch (opt) {
> + case '?':
> + case 'h':
> + errx(1, usage, argv[0]);
This isn't an error and hence wants to produce an exit code of 0.
> + break;
> + default:
> + errx(1, usage, argv[0]);
> + }
Please be consistent with break statements: Either you omit them
uniformly when a noreturn function is call, or you put them there
in all cases.
> + }
>
> if (argc < 2)
> - errx(1, "usage: %s <timeout> <sleep>", argv[0]);
> + errx(1, usage, argv[0]);
>
> daemonize();
>
> h = xc_interface_open(NULL, NULL, 0);
> if (h == NULL)
> - err(1, "xc_interface_open");
> + err(1, "xc_interface_open");
> +
> + t = strtoul(argv[1], &argv[1], 0);
> +
> + // argv1 NaN
NaN is a term normally used for floating point values only.
> + if ( *argv[1] != '\0' )
> + errx(1, "Error: timeout must be a number, got '%s'", argv[1]);
This still doesn't guarantee a numeric: An empty string as argument
would still yield a value of 0 if I'm not mistaken. As would a
string consisting of just blanks.
> - t = strtoul(argv[1], NULL, 0);
> if (t == ULONG_MAX)
> - err(1, "strtoul");
> + err(1, "strtoul");
>
> s = t / 2;
> if (argc == 3) {
> - s = strtoul(argv[2], NULL, 0);
> - if (s == ULONG_MAX)
> - err(1, "strtoul");
> + s = strtoul(argv[2], &argv[2], 0);
> + // argv2 NaN
> + if ( *argv[2] != '\0' ){
> + errx(1, "Error: sleep must be a number, got '%s'", argv[2]);
> + }
Like above you don't really need braces here. If, however, you add
them, the opening one wants to be separated by a blank from the
closing parenthesis. And following other style in this file, there
do not want to be blanks immediately inside the parentheses.
Jan
