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

[Juan Quintela]

"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?


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

> +    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?

Later, Juan.