[RFC,01/18] qemu-storage-daemon: Add barebone tool
diff mbox series

Message ID 20191017130204.16131-2-kwolf@redhat.com
State New
Headers show
Series
  • Add qemu-storage-daemon
Related show

Commit Message

Kevin Wolf Oct. 17, 2019, 1:01 p.m. UTC
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

Comments

Eric Blake Oct. 24, 2019, 1:50 p.m. UTC | #1
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+?


> + */
> +
> +#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?

> +
> +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").

> +        }
> +    }
> +    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.

> +    return ret;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    Error *local_err = NULL;
> +    int ret;
> +
> +#ifdef CONFIG_POSIX
> +    signal(SIGPIPE, SIG_IGN);
> +#endif
> +
> +    error_init(argv[0]);
> +    qemu_init_exec_dir(argv[0]);
> +
> +    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_TRACE);
> +    qemu_add_opts(&qemu_trace_opts);
> +    qcrypto_init(&error_fatal);
> +    bdrv_init();
> +
> +    if (qemu_init_main_loop(&local_err)) {
> +        error_report_err(local_err);
> +        return EXIT_FAILURE;
> +    }
> +
> +    ret = process_options(argc, argv, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
> +}

Quite a trivial shell for now, but looks interesting.  Sadly, I don't 
have much time to review the rest of the series until after KVM Forum, 
which means getting this in (even as experimental) for 4.2 is at risk.
Max Reitz Nov. 6, 2019, 12:11 p.m. UTC | #2
On 17.10.19 15:01, 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
> index 08ca4bcb46..bb3d55fb25 100755
> --- a/configure
> +++ b/configure
> @@ -6034,7 +6034,7 @@ tools=""
>  if test "$want_tools" = "yes" ; then
>    tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) qemu-edid\$(EXESUF) $tools"
>    if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
> -    tools="qemu-nbd\$(EXESUF) $tools"
> +    tools="qemu-nbd\$(EXESUF) qemu-storage-daemon\$(EXESUF) $tools"
>    fi
>    if [ "$ivshmem" = "yes" ]; then
>      tools="ivshmem-client\$(EXESUF) ivshmem-server\$(EXESUF) $tools"
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> new file mode 100644
> index 0000000000..a251dc255c
> --- /dev/null
> +++ 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.
> + */
> +
> +#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>
> +
> +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'},
> +        {0, 0, 0, 0}
> +    };
> +
> +    while ((c = getopt_long(argc, argv, ":hT:V", long_options, NULL)) != -1) {
> +        switch (c) {
> +        case '?':
> +            error_setg(errp, "Unknown option '%s'", argv[optind - 1]);
> +            goto out;

Am I doing something wrong or is optind really not updated when '?' is
returned?

Because I’m getting this:

$ ./qemu-storage-daemon -42
qemu-storage-daemon: Unknown option './qemu-storage-daemon'

But OTOH I also get:

$ ./qemu-img create -42
qemu-img: unrecognized option 'create'
Try 'qemu-img --help' for more information

So, uh, well.

Max
Markus Armbruster Nov. 7, 2019, 4:21 p.m. UTC | #3
In addition to Eric's review:

Kevin Wolf <kwolf@redhat.com> writes:

> 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
> index 08ca4bcb46..bb3d55fb25 100755
> --- a/configure
> +++ b/configure
> @@ -6034,7 +6034,7 @@ tools=""
>  if test "$want_tools" = "yes" ; then
>    tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) qemu-edid\$(EXESUF) $tools"
>    if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
> -    tools="qemu-nbd\$(EXESUF) $tools"
> +    tools="qemu-nbd\$(EXESUF) qemu-storage-daemon\$(EXESUF) $tools"
>    fi
>    if [ "$ivshmem" = "yes" ]; then
>      tools="ivshmem-client\$(EXESUF) ivshmem-server\$(EXESUF) $tools"
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> new file mode 100644
> index 0000000000..a251dc255c
> --- /dev/null
> +++ 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.
> + */

Standard request for new code: please make this GPLv2+, or tell us why
it has to be something else.

> +
> +#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>
> +
> +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'},

You initialize member int *flag with 0 here, ....

> +        {"version", no_argument, 0, 'V'},
> +        {"trace", required_argument, NULL, 'T'},

... and with NULL here.  Recommend to pick one and stick to it.

> +        {0, 0, 0, 0}

{0} or {} would do.

> +    };
> +
> +    while ((c = getopt_long(argc, argv, ":hT:V", long_options, NULL)) != -1) {
> +        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);

This is QemuOpts below the hood.  Fact, not criticism :)

> +            break;
> +        }

Suggest (your preferred variation of) default: assert(0) to catch
omissions.

> +    }
> +    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);

I suspect the only reason for hiding trace initialization within
process_options() is avoiding a global variable @trace_file.  I'd prefer
the global variable over the hiding.

> +
> +    ret = 0;
> +out:
> +    g_free(trace_file);
> +    return ret;
> +}

Since the function exit(0)s on -h and -V anyway, let's exit(1) on error
instead of mucking around with error_setg().  You can then leave
reporting unknown options and missing option arguments to getopt_long().
Saves you the trouble of fixing the bug Max pointed out.

> +
> +int main(int argc, char *argv[])
> +{
> +    Error *local_err = NULL;
> +    int ret;
> +
> +#ifdef CONFIG_POSIX
> +    signal(SIGPIPE, SIG_IGN);
> +#endif
> +
> +    error_init(argv[0]);
> +    qemu_init_exec_dir(argv[0]);
> +
> +    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_TRACE);
> +    qemu_add_opts(&qemu_trace_opts);
> +    qcrypto_init(&error_fatal);
> +    bdrv_init();

Out of curiosity: how did you come up with this set of initializations?

> +
> +    if (qemu_init_main_loop(&local_err)) {
> +        error_report_err(local_err);
> +        return EXIT_FAILURE;

What's wrong with

       qemu_init_main_loop(&error_fatal)

?

> +    }
> +
> +    ret = process_options(argc, argv, &local_err);
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
> +}
> diff --git a/Makefile b/Makefile
> index 30f0abfb42..76338d0ab4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -558,6 +558,7 @@ qemu-img.o: qemu-img-cmds.h
>  qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
> +qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
>  
>  qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)
Kevin Wolf Nov. 13, 2019, 2:12 p.m. UTC | #4
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

Patch
diff mbox series

diff --git a/configure b/configure
index 08ca4bcb46..bb3d55fb25 100755
--- a/configure
+++ b/configure
@@ -6034,7 +6034,7 @@  tools=""
 if test "$want_tools" = "yes" ; then
   tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) qemu-edid\$(EXESUF) $tools"
   if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
-    tools="qemu-nbd\$(EXESUF) $tools"
+    tools="qemu-nbd\$(EXESUF) qemu-storage-daemon\$(EXESUF) $tools"
   fi
   if [ "$ivshmem" = "yes" ]; then
     tools="ivshmem-client\$(EXESUF) ivshmem-server\$(EXESUF) $tools"
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
new file mode 100644
index 0000000000..a251dc255c
--- /dev/null
+++ 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.
+ */
+
+#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>
+
+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'},
+        {0, 0, 0, 0}
+    };
+
+    while ((c = getopt_long(argc, argv, ":hT:V", long_options, NULL)) != -1) {
+        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;
+        }
+    }
+    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);
+    return ret;
+}
+
+int main(int argc, char *argv[])
+{
+    Error *local_err = NULL;
+    int ret;
+
+#ifdef CONFIG_POSIX
+    signal(SIGPIPE, SIG_IGN);
+#endif
+
+    error_init(argv[0]);
+    qemu_init_exec_dir(argv[0]);
+
+    module_call_init(MODULE_INIT_QOM);
+    module_call_init(MODULE_INIT_TRACE);
+    qemu_add_opts(&qemu_trace_opts);
+    qcrypto_init(&error_fatal);
+    bdrv_init();
+
+    if (qemu_init_main_loop(&local_err)) {
+        error_report_err(local_err);
+        return EXIT_FAILURE;
+    }
+
+    ret = process_options(argc, argv, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
+}
diff --git a/Makefile b/Makefile
index 30f0abfb42..76338d0ab4 100644
--- a/Makefile
+++ b/Makefile
@@ -558,6 +558,7 @@  qemu-img.o: qemu-img-cmds.h
 qemu-img$(EXESUF): qemu-img.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-nbd$(EXESUF): qemu-nbd.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 qemu-io$(EXESUF): qemu-io.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
+qemu-storage-daemon$(EXESUF): qemu-storage-daemon.o $(authz-obj-y) $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)