Re: [Qemu-devel] [PATCH v3 04/15] monitor: remove usage of generated marshal functions

[Marc-André Lureau]

Hi

On Tue, Aug 9, 2016 at 12:38 PM Markus Armbruster <address@hidden> wrote:

> address@hidden writes:
>
> > From: Marc-André Lureau <address@hidden>
> >
> > Once the middle mode is removed, the generated marshal functions will no
> > longer be exported.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> >  monitor.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index c9191e5..3a4a533 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3659,21 +3659,21 @@ static int monitor_can_read(void *opaque)
> >      return (mon->suspend_cnt == 0) ? 1 : 0;
> >  }
> >
> > -static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
> > +static bool invalid_qmp_mode(const Monitor *mon, const char *cmd,
> >                               Error **errp)
> >  {
> > -    bool is_cap = cmd->mhandler.cmd_new == qmp_marshal_qmp_capabilities;
> > +    bool is_cap = g_str_equal(cmd, "qmp_capabilities");
>
> We usually do !strcmp(...).  The negation is somewhat error-prone, which
> is why some people argue for using g_str_equal() instead.  Okay.
>
> >
> >      if (is_cap && mon->qmp.in_command_mode) {
> >          error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> >                    "Capabilities negotiation is already complete,
> command "
> > -                  "'%s' ignored", cmd->name);
> > +                  "'%s' ignored", cmd);
> >          return true;
> >      }
> >      if (!is_cap && !mon->qmp.in_command_mode) {
> >          error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> >                    "Expecting capabilities negotiation with "
> > -                  "'qmp_capabilities' before command '%s'", cmd->name);
> > +                  "'qmp_capabilities' before command '%s'", cmd);
> >          return true;
> >      }
> >      return false;
> > @@ -3964,7 +3964,7 @@ static void handle_qmp_command(JSONMessageParser
> *parser, GQueue *tokens)
> >                    "The command %s has not been found", cmd_name);
> >          goto err_out;
> >      }
> > -    if (invalid_qmp_mode(mon, cmd, &local_err)) {
> > +    if (invalid_qmp_mode(mon, cmd_name, &local_err)) {
> >          goto err_out;
> >      }
>
> Patch is okay, but if we decide to keep things simple and give all
> marshallers external linkage, it's unnecessary.
>

I would rather avoid touching marshaller from outside.

It also avoids having to lookup the marshaller twice in
handle_qmp_command(). (or we would need to teach qmp_dispatch() to take an
already looked up marshaller). Well, that's not really bad but still worth
to mention.
-- 
Marc-André Lureau