Re: [Qemu-devel] [PATCH 3/3] qapi: convert add_client

[Luiz Capitulino]

On Tue, 25 Sep 2012 13:29:32 +0200
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > Also fixes a few issues while there:
> >
> >  1. The fd returned by monitor_get_fd() leaks in most error conditions
> >  2. monitor_get_fd() return value is not checked. Best case we get
> >     an error that is not correctly reported, worse case one of the
> >     functions using the fd (with value of -1) will explode
> >  3. A few error conditions aren't reported
> 
> 4. We now "use up" @fdname always.  Before, it was left alone for
>    invalid @protocol.

By "uses up" you mean that the fd will be consumed from the monitor's
poll? I guess that's true for every command that accepts fds.

> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  monitor.c        | 39 ---------------------------------------
> >  qapi-schema.json | 23 +++++++++++++++++++++++
> >  qmp-commands.hx  |  5 +----
> >  qmp.c            | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 67 insertions(+), 43 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 645f8f4..e18df38 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -944,45 +944,6 @@ static void do_trace_print_events(Monitor *mon)
> >      trace_print_events((FILE *)mon, &monitor_fprintf);
> >  }
> >  
> > -static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject 
> > **ret_data)
> > -{
> > -    const char *protocol  = qdict_get_str(qdict, "protocol");
> > -    const char *fdname = qdict_get_str(qdict, "fdname");
> > -    CharDriverState *s;
> > -
> > -    if (strcmp(protocol, "spice") == 0) {
> > -        int fd = monitor_get_fd(mon, fdname, NULL);
> > -        int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> > -        int tls = qdict_get_try_bool(qdict, "tls", 0);
> > -        if (!using_spice) {
> > -            /* correct one? spice isn't a device ,,, */
> > -            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
> > -            return -1;
> > -        }
> > -        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
> > -            close(fd);
> > -        }
> > -        return 0;
> > -#ifdef CONFIG_VNC
> > -    } else if (strcmp(protocol, "vnc") == 0) {
> > -   int fd = monitor_get_fd(mon, fdname, NULL);
> > -        int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> > -   vnc_display_add_client(NULL, fd, skipauth);
> > -   return 0;
> > -#endif
> > -    } else if ((s = qemu_chr_find(protocol)) != NULL) {
> > -   int fd = monitor_get_fd(mon, fdname, NULL);
> > -   if (qemu_chr_add_client(s, fd) < 0) {
> > -       qerror_report(QERR_ADD_CLIENT_FAILED);
> > -       return -1;
> > -   }
> > -   return 0;
> > -    }
> > -
> > -    qerror_report(QERR_INVALID_PARAMETER, "protocol");
> > -    return -1;
> > -}
> > -
> >  static int client_migrate_info(Monitor *mon, const QDict *qdict,
> >                                 MonitorCompletion cb, void *opaque)
> >  {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 14e4419..c977ec7 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -33,6 +33,29 @@
> >              'MigrationExpected' ] }
> >  
> >  ##
> > +# @add_client
> > +#
> > +# Allow client connections for VNC, Spice and socket based
> > +# character devices to be passed in to QEMU via SCM_RIGHTS.
> > +#
> > +# @protocol: protocol name. Valid names are "vnc", "spice" or the
> > +#            name of a character device (eg. from -chardev id=XXXX)
> 
> Not this patch's fault, but here goes anyway: rotten design, breaks when
> you name your character device "vnc" or "spice".  I think we shouldn't
> overload commands like that.

Agreed, I think a better design would be to have separate commands.

> > +#
> > +# @fdname: file descriptor name previously passed via 'getfd' command
> > +#
> > +# @skipauth: #optional whether to skip authentication
> 
> Should we document that this applies only to vnc and spice?
> 
> > +#
> > +# @tls: #optional whether to perform TLS
> 
> Should we document that this applies only to spice?
> 
> > +#
> > +# Returns: nothing on success.
> > +#
> > +# Since: 0.14.0
> > +##
> 
> Should we document that this always "uses up" @fdname, i.e. even when it
> fails?

Yes, as I'll have to respin 2/3 anyway...

> > +{ 'command': 'add_client',
> > +  'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
> > +            '*tls': 'bool' } }
> > +
> > +##
> >  # @NameInfo:
> >  #
> >  # Guest name information.
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 6e21ddb..36e08d9 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -1231,10 +1231,7 @@ EQMP
> >      {
> >          .name       = "add_client",
> >          .args_type  = "protocol:s,fdname:s,skipauth:b?,tls:b?",
> > -        .params     = "protocol fdname skipauth tls",
> > -        .help       = "add a graphics client",
> > -        .user_print = monitor_user_noop,
> > -        .mhandler.cmd_new = add_graphics_client,
> > +        .mhandler.cmd_new = qmp_marshal_input_add_client,
> >      },
> >  
> >  SQMP
> > diff --git a/qmp.c b/qmp.c
> > index 8463922..36c54c5 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -479,3 +479,46 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
> > **errp)
> >      return arch_query_cpu_definitions(errp);
> >  }
> >  
> > +void qmp_add_client(const char *protocol, const char *fdname,
> > +                    bool has_skipauth, bool skipauth, bool has_tls, bool 
> > tls,
> > +                    Error **errp)
> > +{
> > +    CharDriverState *s;
> > +    int fd;
> > +
> > +    fd = monitor_get_fd(cur_mon, fdname, errp);
> > +    if (fd < 0) {
> > +        return;
> > +    }
> > +
> > +    if (strcmp(protocol, "spice") == 0) {
> > +        if (!using_spice) {
> > +            error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
> > +            close(fd);
> > +            return;
> > +        }
> > +        skipauth = has_skipauth ? skipauth : false;
> > +        tls = has_tls ? tls : false;
> 
> Pattern "has_FOO ? FOO : DEFAULT".  Would it be feasible and useful to
> declare the default in the schema, and pass only FOO to the function
> then, not has_FOO?  Outside this patch's scope, of course.

I think so.

> > +        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
> > +            error_setg(errp, "spice failed to add client");
> 
> Error message could use some love, but anything is an improvement over
> nothing.

I actually tried to find information whether spice_server_add_ssl_client()
and spice_server_add_client() set errno on failure, but I remember I didn't
find any official doc. Maybe I didn't look for it correctly, will try again.

But in case no doc clearly states that we can read errno on failure, then
I'll prefer the generic error.

> > +            close(fd);
> > +        }
> > +        return;
> > +#ifdef CONFIG_VNC
> > +    } else if (strcmp(protocol, "vnc") == 0) {
> 
> I hate "return; else", but to each his own :)
> 
> > +        skipauth = has_skipauth ? skipauth : false;
> > +        vnc_display_add_client(NULL, fd, skipauth);
> 
> Amazingly, this can't fail.  Wonder what happens when you pass a bogus
> file descriptor...  Not this patch's fault.
> 
> > +        return;
> > +#endif
> > +    } else if ((s = qemu_chr_find(protocol)) != NULL) {
> > +        if (qemu_chr_add_client(s, fd) < 0) {
> > +            error_setg(errp, "failed to add client");
> 
> Error message could use some love, but it's no worse than before.

Today, this can't fail. The only chardev that defines this method
is the socket one and it can't fail.

> 
> > +            close(fd);
> > +            return;
> > +        }
> > +        return;
> > +    }
> > +
> > +    error_setg(errp, "protocol '%s' is invalid", protocol);
> > +    close(fd);
> > +}
> 
> Just comments, no objections.
>