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

On Tue, Apr 16, 2013 at 3:49 PM, Thomas Bächler <[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.

Thanks for the suggestions Thomas. I implemented most of them, so now
it should be a lot simpler to extend this in the future.

The only thing I skipped was the --format=? idea, as this info is
already in the help output, and I'm not sure how useful it is to
parse. That said, it could easily be added follow-up patch if there is
a need for it.

-t
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html