Re: [Qemu-devel] [PATCH v2 4/8] s390x: Dump storage keys qmp command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5908 bytes --]

On 08/25/2015 10:10 AM, Cornelia Huck wrote:
> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> 
> Provide a dump-skeys qmp command to allow the end user to dump storage
> keys. This is useful for debugging problems with guest storage key support
> within Qemu and for guest operating system developers.
> 
> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---

>  
> +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
> +                       uint64_t count, Error **errp)
> +{
> +    uint64_t curpage = startgfn;
> +    uint64_t maxpage = curpage + count - 1;
> +    const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
> +                      " ch=%d, reserved=%d\n";
> +    char *buf = g_try_malloc(128);
> +    int len;
> +
> +    if (!buf) {
> +        error_setg(errp, "Out of memory");
> +        return;
> +    }

128 bytes is small enough to just stack-allocate, and forget about
malloc().  Even if you insist on malloc'ing, a simple g_malloc() is
nicer than g_try_malloc(), as it is unlikely to fail (and if it DOES
fail, something else is likely to fail soon) - we tend to reserve
g_try_malloc() for potentially large allocations where failure is more
likely.

> +
> +    for (; curpage <= maxpage; curpage++) {
> +        uint8_t acc = (*keys & 0xF0) >> 4;
> +        int fp =  (*keys & 0x08);
> +        int ref = (*keys & 0x04);
> +        int ch = (*keys & 0x02);
> +        int res = (*keys & 0x01);
> +
> +        len = snprintf(buf, 128, fmt, curpage,

If you stack-allocate buf, then sizeof(buf) is nicer than hard-coded 128
here.

> +                       *keys, acc, fp, ref, ch, res);
> +        qemu_put_buffer(f, (uint8_t *)buf, len);

Potential bug. snprintf() returns how many bytes WOULD have been printed
if the buffer is large enough, and may therefore be larger than 128 if
your buffer size guess was wrong or the format string is edited.  The
only way to safely use snprintf is to first check that the result is no
larger than the input, before passing the string on to qemu_put_buffer().

> +void qmp_dump_skeys(const char *filename, Error **errp)
> +{
> +    S390SKeysState *ss = s390_get_skeys_device();
> +    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
> +    const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
> +    uint64_t handled_count = 0, cur_count;
> +    Error *lerr = NULL;
> +    vaddr cur_gfn = 0;
> +    uint8_t *buf;
> +    int ret;
> +    QEMUFile *f;
> +
> +    /* Quick check to see if guest is using storage keys*/
> +    if (!skeyclass->skeys_enabled(ss)) {
> +        error_setg(&lerr, "This guest is not using storage keys. "
> +                         "Nothing to dump.");

Error messages don't usually end in '.'

> +        error_propagate(errp, lerr);

Instead of setting the local error just to propagate it, just write the
error message directly into errp, as in:

error_setg(errp, ...)

> +        return;
> +    }
> +
> +    f = qemu_fopen(filename, "wb");
> +    if (!f) {
> +        error_setg(&lerr, "Could not open file");
> +        error_propagate(errp, lerr);

Same story. Also, we have error_setg_file_open() which is more
appropriate to use here.

> +        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
> +        if (ret < 0) {
> +            error_setg(&lerr, "get_keys error %d", ret);
> +            error_propagate(errp, lerr);
> +            goto out_free;
> +        }
> +
> +        /* write keys to stream */
> +        write_keys(f, buf, cur_gfn, cur_count, &lerr);
> +        if (lerr) {
> +            error_propagate(errp, lerr);
> +            goto out_free;

Instead of propagating the error on every caller...

> +        }
> +
> +        cur_gfn += cur_count;
> +        handled_count += cur_count;
> +    }
> +
> +out_free:
> +    g_free(buf);

you could do it just once here unconditionally (it is safe to call
error_propagate(..., NULL) when no error occurred).

> +++ b/qapi-schema.json
> @@ -2058,6 +2058,19 @@
>    'returns': 'DumpGuestMemoryCapability' }
>  
>  ##
> +# @dump-skeys
> +#
> +# Dump guest's storage keys.  @filename: the path to the file to dump to.

Newline before @filename, please.

> +# This command is only supported on s390 architecture.

It would be nice if we fixed the qapi generator to allow conditional
compilation of the .json files, so that the command is not even exposed
on other platforms.  Markus mentioned that at KVM Forum as one of the
possible followups to pursue after his current pending series on
introspection lands. [1]

> +#
> +# Returns: nothing on success

The 'Returns' line adds no information, so it is better omitted.

> +#
> +# Since: 2.5
> +##
> +{ 'command': 'dump-skeys',
> +  'data': { 'filename': 'str' } }
> +
> +##
>  # @netdev_add:
>  #
>  # Add a network backend.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba630b1..9848fd8 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -872,6 +872,31 @@ Example:
>  
>  EQMP
>  
> +#if defined TARGET_S390X
> +    {
> +        .name       = "dump-skeys",
> +        .args_type  = "filename:F",
> +        .mhandler.cmd_new = qmp_marshal_input_dump_skeys,
> +    },
> +#endif

[1] At any rate, as long as we have the .hx file that does support
conditional compilation, I think 'query-commands' properly shows whether
the command is present, even if Markus' addition of 'query-schema' does
not have the same luxury of omitting unused stuff.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]