Re: [PATCH v3] tools: add static-nodes tool

On Tue, Apr 16, 2013 at 10:49 AM, Thomas Bächler <thomas-fd97jBR+K/[email protected]> wrote:
> Am 16.04.2013 15:12, schrieb Tom Gundersen:
>> +static void write_human(FILE *out, char module[], char devname[], char type, unsigned int maj, unsigned int min)
>
> [...]
>
>> +static void write_tmpfile(FILE *out, char devname[], char type, unsigned int maj, unsigned int min)
>
> [...]
>
>> +static int do_static_nodes(int argc, char *argv[])
>> +{
>> +        struct utsname kernel;
>> +        char modules[PATH_MAX];
>> +        FILE *in = NULL, *out = stdout;
>> +        bool human_readable = 1;
>
> This code emphasizes that there is actually only one format available
> and needs to be changed again when another one is added. Why not
>
> void (*write_output)((FILE *, char[], char[], char, unsigned int,
> unsigned int) = write_human;
>
> ? Then ...
>
>> +                case 'f':
>> +                        if (!streq(optarg, "tmpfiles")) {
>> +                                fprintf(stderr, "Unknown format: '%s'.\n", argv[1]);
>> +                                help();
>> +                                ret = EXIT_FAILURE;
>> +                                goto finish;
>> +                        }
>> +                        human_readable = 0;
>> +                        break;
>
> case 'f':
>     if (streq(optarg, "tmpfiles")) {
>          write_output = write_tmpfiles;
>     }
>     else {
>         fprintf(stderr, "Unknown format: '%s'.\n", argv[1]);
>         [...]
>     }
>     break;
>
> And in the end:
>
>> +                if (human_readable)
>> +                        write_human(out, module, devname, type, maj, min);
>> +                else
>> +                        write_tmpfile(out, devname, type, maj, min);
>
> write_output(out, module, devname, type, maj, min);
>
> Maybe even add an array with output name and function pointer pairs, so
> that we could get a list of available formats using --format=?. For
> consistency, --format=human should also work. Just seems nicer to me, in
> case someone actually plans to extend this later.
>

Agree. Otherwise looks good.

Kay, what's your opinion regarding this command? Is this sufficient to
drop CAP_MKNOD from udev?


Lucas De Marchi