Re: [Qemu-devel] [PATCH 3/3] Add optional migrate version to hmp migrate command

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > Uses the new 'optional parameter with string' parameter type
> >
> > e.g.
> > migrate -v "2.0.0 (foo)" "exec: whatever"
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> 
> 
> > diff --git a/hmp.c b/hmp.c
> > index 411e4bc..e06d8c9 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1272,18 +1272,122 @@ static void hmp_migrate_status_cb(void *opaque)
> >      qapi_free_MigrationInfo(info);
> >  }
> >  
> > +/* Parse a string into a VersionInfo structure
> > + * if the input string is NULL, return NULL.
> > + * The input is the same as that output by 'info version'
> > + * example input:
> > + *    3.1.4 (arbitrary-package-name-3.1.4-stuff)
> > + *    2.0.0
> > + */
> 
> This function is similar to qmp_query_version(), could we refactor them
> to share the same code?

See below.

> > +static VersionInfo *parse_version_info(Monitor *mon, const char *s)
> > +{
> > +    char *tmps;
> > +    VersionInfo *vi = NULL;
> > +    const char *err_text;
> > +
> > +    if (!s) {
> > +        return NULL;
> > +    }
> > +
> > +    if (!*s) {
> > +        err_text = "Empty version string";
> > +        goto badstring;
> > +    }
> > +
> > +    /*
> > +     * There must be an easier way; but scanf's %n is apparently not
> > +     * portable to check if we hit the end of the string after the numeric
> > +     * part.
> > +     */
> > +    vi = g_malloc(sizeof(VersionInfo));
> > +    errno = 0;
> > +    if (!isdigit(*s)) {
> > +        err_text = "Parsing major version";
> > +        goto badstring;
> > +    }
> > +    vi->qemu.major = (int)strtol(s, &tmps, 10);
> 
> qemu_query_version() don't need this cast.
> And it has much less error checking.

Yes I think we might be able to share some of it with qemu_query_version
if you think it's worth it.  This code is written to be quite picky since the
text is coming from the user.
And yes, you're right those casts can go.

> > +    if (errno || (*tmps != '.')) {
> > +        err_text = "Parsing major version";
> > +        goto badstring;
> > +    }
> > +    if (!isdigit(*(++tmps))) {
> > +        err_text = "Parsing minor version";
> > +        goto badstring;
> > +    }
> > +    vi->qemu.minor = (int)strtol(tmps, &tmps, 10);
> > +    if (errno || (*tmps != '.')) {
> > +        err_text = "Parsing minor version";
> > +        goto badstring;
> > +    }
> > +    if (!isdigit(*(++tmps))) {
> > +        err_text = "Parsing micro version";
> > +        goto badstring;
> > +    }
> > +    vi->qemu.micro = (int)strtol(tmps, &tmps, 10);
> > +    if (errno) {
> > +        err_text = "Parsing micro version";
> > +        goto badstring;
> > +    }
> > +
> > +    /*
> > +     * At this point we're either at the end (fine) or we should have a
> > +     * version in brackets
> > +     */
> > +    if (*tmps) {
> > +        /* Expect a (package version) */
> > +        char *open_bracket = strchr(tmps, '(');
> > +        if (!open_bracket) {
> > +            err_text = "Finding open bracket";
> > +            goto badstring;
> > +        }
> > +        char *close_bracket = strchr(open_bracket+1, ')');
> > +        if (!close_bracket) {
> > +            err_text = "Finding close bracket";
> > +            goto badstring;
> > +        }
> > +
> > +        tmps = g_strdup(open_bracket+1);
> > +        *strchr(tmps, ')') = '\0';
> > +        vi->package = tmps;
> > +    } else {
> > +        /* Just makes everything consistent later */
> > +        vi->package = g_malloc(1);
> > +        vi->package[0] = '\0';
> > +    }
> > +
> > +    return vi;
> > +
> > +badstring:
> > +    monitor_printf(mon, "Failed to parse version (%s)\n", err_text);
> > +    if (vi) {
> > +        g_free(vi);
> > +    }
> > +    return NULL;
> > +}
> 
> Only thing that I can think about is that we could set the destination
> version as a migration capability.  Just a thought, don't know if it
> would be better/worse.  What do you think?

When I originally thought about this I was going to make it a capability,
and Daniel Berrange wondered why I hadn't made it a parameter to migrate :-)
I realised that if I made it a capability, the caller would have to be more
careful when doing a 2nd migration (e.g. after a failed migration) otherwise
there was a risk of the old version still being used.  As a parameter to
'migrate' it's obviously safe against that type of bug.

Also, all the existing capabilities are simple numbers, so this seemed easier
than having to rework that.

Dave

> Later, Juan.
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK