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

[Jason J. Herne]

On 08/25/2015 12:51 PM, Eric Blake wrote:
> On 08/25/2015 10:10 AM, Cornelia Huck wrote:
> > From: "Jason J. Herne" <address@hidden>
> >
> > 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 <address@hidden>
> > Reviewed-by: David Hildenbrand <address@hidden>
> > Signed-off-by: Jason J. Herne <address@hidden>
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
>
> > +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.

Will do.

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

Will use sizeof()

> > +                       *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().

I chose 128 because it is large enough to handle even the most extreme 
cases.
I understand that someone could change the format string later on but if 
that
is the case then they should update the buffer size accordingly. I can add a
comment to that effect if you think that is good. But I'd like to avoid
over engineering something that is fairly simple.

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

Will fix.

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

Ok, will fix.

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

Neat :) Will switch to that.

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

Awesome, thanks for the tip.

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

Will fix.

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

Will fix.

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

--
-- Jason J. Herne (address@hidden)