Re: [RFC PATCH 01/18] qemu-storage-daemon: Add barebone tool

[Kevin Wolf <kwolf@redhat.com>]

Am 24.10.2019 um 15:50 hat Eric Blake geschrieben:
> On 10/17/19 8:01 AM, Kevin Wolf wrote:
> > This adds a new binary qemu-storage-daemon that doesn't yet do more than
> > some typical initialisation for tools and parsing the basic command
> > options --version, --help and --trace.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   configure             |   2 +-
> >   qemu-storage-daemon.c | 141 ++++++++++++++++++++++++++++++++++++++++++
> >   Makefile              |   1 +
> >   3 files changed, 143 insertions(+), 1 deletion(-)
> >   create mode 100644 qemu-storage-daemon.c
> > 
> > diff --git a/configure b/configure
> 
> > +++ b/qemu-storage-daemon.c
> > @@ -0,0 +1,141 @@
> > +/*
> > + * QEMU storage daemon
> > + *
> > + * Copyright (c) 2019 Kevin Wolf <kwolf@redhat.com>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> 
> Is there an intent to license this binary as BSD (by restricting sources
> that can be linked in), or is it going to end up as GPLv2+ for other
> reasons? If the latter, would it be better to license this file GPLv2+?

The binary will be GPLv2 either way. Maybe even GPLv2+, not sure about
that.

This file copies quite a bit from qemu-img.c and vl.c, so I'm using the
same license as there. Of course, the "bug" in my patch is that I also
need to keep not only the license text, but also the actual copyright
line from there. Will fix.

> 
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "block/block.h"
> > +#include "crypto/init.h"
> > +
> > +#include "qapi/error.h"
> > +#include "qemu-common.h"
> > +#include "qemu-version.h"
> > +#include "qemu/config-file.h"
> > +#include "qemu/error-report.h"
> > +#include "qemu/log.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/module.h"
> > +
> > +#include "trace/control.h"
> > +
> > +#include <getopt.h>
> 
> Shouldn't system headers appear right after qemu/osdep.h?

I wasn't aware of this, but CODING_STYLE.rst agrees with you.

> > +
> > +static void help(void)
> > +{
> > +    printf(
> > +"Usage: %s [options]\n"
> > +"QEMU storage daemon\n"
> > +"\n"
> > +"  -h, --help             display this help and exit\n"
> > +"  -T, --trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n"
> > +"                         specify tracing options\n"
> > +"  -V, --version          output version information and exit\n"
> > +"\n"
> > +QEMU_HELP_BOTTOM "\n",
> > +    error_get_progname());
> > +}
> > +
> > +static int process_options(int argc, char *argv[], Error **errp)
> > +{
> > +    int c;
> > +    char *trace_file = NULL;
> > +    int ret = -EINVAL;
> > +
> > +    static const struct option long_options[] = {
> > +        {"help", no_argument, 0, 'h'},
> > +        {"version", no_argument, 0, 'V'},
> > +        {"trace", required_argument, NULL, 'T'},
> 
> I find it harder to maintain lists of options (which will get longer over
> time) when the order of the struct...
> 
> > +        {0, 0, 0, 0}
> > +    };
> > +
> > +    while ((c = getopt_long(argc, argv, ":hT:V", long_options, NULL)) != -1) {
> 
> ...the order of the short options...
> 
> > +        switch (c) {
> > +        case '?':
> > +            error_setg(errp, "Unknown option '%s'", argv[optind - 1]);
> > +            goto out;
> > +        case ':':
> > +            error_setg(errp, "Missing option argument for '%s'",
> > +                       argv[optind - 1]);
> > +            goto out;
> > +        case 'h':
> > +            help();
> > +            exit(EXIT_SUCCESS);
> > +        case 'V':
> > +            printf("qemu-storage-daemon version "
> > +                   QEMU_FULL_VERSION "\n" QEMU_COPYRIGHT "\n");
> > +            exit(EXIT_SUCCESS);
> > +        case 'T':
> > +            g_free(trace_file);
> > +            trace_file = trace_opt_parse(optarg);
> > +            break;
> 
> ...and the order of the case statements are not identical. Case-insensitive
> alphabetical may be easiest (matching your shortopt ordering of ":hT:V").

Makes sense. (I think I tried to remember to keep things in alphabetical
order at least sometimes, but apparently I didn't try hard enough.)

> > +        }
> > +    }
> > +    if (optind != argc) {
> > +        error_setg(errp, "Unexpected argument: %s", argv[optind]);
> > +        goto out;
> > +    }
> > +
> > +    if (!trace_init_backends()) {
> > +        error_setg(errp, "Could not initialize trace backends");
> > +        goto out;
> > +    }
> > +    trace_init_file(trace_file);
> > +    qemu_set_log(LOG_TRACE);
> > +
> > +    ret = 0;
> > +out:
> > +    g_free(trace_file);
> 
> Mark trace_file as g_auto, and you can avoid the out: label altogether.

Oooh, magic! :-)

Kevin