Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

[Eric Blake <eblake@redhat.com>]

On Mon, Oct 11, 2021 at 04:03:01PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Reiter (s.reiter@proxmox.com) wrote:
> > It is possible to specify more than one VNC server on the command line,
> > either with an explicit ID or the auto-generated ones à la "default",
> > "vnc2", "vnc3", ...
> > 

> > +++ b/monitor/hmp-cmds.c
> > @@ -1451,10 +1451,41 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *protocol  = qdict_get_str(qdict, "protocol");
> >      const char *password  = qdict_get_str(qdict, "password");
> > +    const char *display = qdict_get_try_str(qdict, "display");
> >      const char *connected = qdict_get_try_str(qdict, "connected");
> >      Error *err = NULL;
> > +    DisplayProtocol proto;
> >  
> > -    qmp_set_password(protocol, password, !!connected, connected, &err);
> > +    SetPasswordOptions opts = {
> > +        .password = g_strdup(password),
> 
> You're leaking that strdup on the error returns; you probably want to
> add an error:  exit and goto it to do all the cleanup.

Or maybe there's a way to use g_autofree to let the compiler clean it
up automatically.

> 
> > +        .u.vnc.display = NULL,
> > +    };
> > +
> > +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> > +                            DISPLAY_PROTOCOL_VNC, &err);
> > +    if (err) {
> > +        hmp_handle_error(mon, err);
> > +        return;
> > +    }
> > +    opts.protocol = proto;
> > +
> > +    if (proto == DISPLAY_PROTOCOL_VNC) {
> > +        opts.u.vnc.has_display = !!display;
> > +        opts.u.vnc.display = g_strdup(display);
> > +    } else if (proto == DISPLAY_PROTOCOL_SPICE) {
> > +        opts.u.spice.has_connected = !!connected;
> > +        opts.u.spice.connected =
> > +            qapi_enum_parse(&SetPasswordAction_lookup, connected,
> > +                            SET_PASSWORD_ACTION_KEEP, &err);
> > +        if (err) {
> > +            hmp_handle_error(mon, err);
> > +            return;
> > +        }
> > +    }
> > +
> > +    qmp_set_password(&opts, &err);
> > +    g_free(opts.password);
> > +    g_free(opts.u.vnc.display);

Hmm. Why are we hand-cleaning only portions of the QAPI type instead
of using the generated qapi_free_SetPasswordOptions() do to things?

> >      hmp_handle_error(mon, err);
> >  }
> >  
> > @@ -1462,9 +1493,31 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *protocol  = qdict_get_str(qdict, "protocol");
> >      const char *whenstr = qdict_get_str(qdict, "time");
> > +    const char *display = qdict_get_try_str(qdict, "display");
> >      Error *err = NULL;
> > +    DisplayProtocol proto;
> >  
> > -    qmp_expire_password(protocol, whenstr, &err);
> > +    ExpirePasswordOptions opts = {
> > +        .time = g_strdup(whenstr),
> > +        .u.vnc.display = NULL,
> > +    };
> 
> Same here; that 'whenstr' gets leaked on errors.
> 
> > +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> > +                            DISPLAY_PROTOCOL_VNC, &err);
> > +    if (err) {
> > +        hmp_handle_error(mon, err);
> > +        return;
> > +    }
> > +    opts.protocol = proto;
> > +
> > +    if (proto == DISPLAY_PROTOCOL_VNC) {
> > +        opts.u.vnc.has_display = !!display;
> > +        opts.u.vnc.display = g_strdup(display);
> > +    }
> > +
> > +    qmp_expire_password(&opts, &err);
> > +    g_free(opts.time);
> > +    g_free(opts.u.vnc.display);
> >      hmp_handle_error(mon, err);

Same here - using the generated qapi_free_ function rather than doing
things by hand, and/or smart use of g_autofree, may make this easier
to maintain.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org