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 <kw...@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 <kw...@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
