Re: [PATCH 1/3] nbd: Simplify meta-context parsing

[Eric Blake]

On 9/26/20 7:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.09.2020 23:32, Eric Blake wrote:
> > We had a premature optimization of trying to read as little from the
> > wire as possible while handling NBD_OPT_SET_META_CONTEXT in phases.
> > But in reality, we HAVE to read the entire string from the client
> > before we can get to the next command, and it is easier to just read
> > it all at once than it is to read it in pieces.  And once we do that,
> > several functions end up no longer performing I/O, and no longer need
> > to return a value.
> >
> > While simplifying this, take advantage of g_str_has_prefix for less
> > repetition of boilerplate string length computation.
> >
> > Our iotests still pass; I also checked that libnbd's testsuite (which
> > covers more corner cases of odd meta context requests) still passes.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---

> > -/* Read strlen(@pattern) bytes, and set @match to true if they match 
> > @pattern.
> > - * @match is never set to false.
> > - *
> > - * Return -errno on I/O error, 0 if option was completely handled by
> > - * sending a reply about inconsistent lengths, or 1 on success.
> > - *
> > - * Note: return code = 1 doesn't mean that we've read exactly @pattern.
> > - * It only means that there are no errors.
> > +
> > +/*
> > + * Check @ns with @len bytes, and set @match to true if it matches 
> > @pattern,
> > + * or if @len is 0 and the client is performing _LIST_. @match is 
> > never set
> > + * to false.
> >    */
> > -static int nbd_meta_pattern(NBDClient *client, const char *pattern, 
> > bool *match,
> > -                            Error **errp)
> > +static void nbd_meta_empty_or_pattern(NBDClient *client, const char 
> > *pattern,
> > +                                      const char *ns, uint32_t len,
>
> ns changed its meaning, it's not just a namespace, but the whole query. 
> I think, better to rename it.

Sure, that makes sense.

> Also, it's unusual to pass length together with nul-terminated string, 
> it seems redundant.
> And, it's used only to compare with zero, strlen(ns) == 0 or ns[0] == 0 
> is not slower.

Good point.

> Also, errp is unused argument. And it violate Error API recommendation 
> to not create void functions with errp.

Another simplification.  Looks like I'll be spinning v2.

> Also we can use bool return instead of return through pointer.

That changes the callers, but it's not necessarily a bad thing; whereas 
we were doing:

if (cond) {
    nbd_meta_pattern(..., &match, ...);
}

which either leaves match unchanged or sets it to true, we could instead do:

if (nbd_meta_pattern(...)) {
    match = true;
}

My only worry is that the more changes I make, the harder the patch is 
to read.  I don't know if it's worth breaking it into steps, or just 
doing all the simplifications in one blow.


> > +static void nbd_meta_base_query(NBDClient *client, 
> > NBDExportMetaContexts *meta,
> > +                                const char *ns, uint32_t len, Error 
> > **errp)
> >   {
> > -    return nbd_meta_empty_or_pattern(client, "allocation", len,
> > -                                     &meta->base_allocation, errp);
> > +    nbd_meta_empty_or_pattern(client, "allocation", ns + 5, len - 5,
>
> This one is correct, but would be simpler, if only subquery (after 
> colon) passed here.
> (Really, no reason to pass 'base:' to _base_ func)
>
> Hmm, I see that it gives a possibility to use 
> meta->exp->export_bitmap_context directly.

Yeah, that's where it got interesting.  A direct strcmp() is nice, but 
if we strip a prefix in one place, we have to strip it in the other.  I 
could go either way.


> > + * @len is the length of @ns, including the `qemu:' prefix.
> > + */
> > +static void nbd_meta_qemu_query(NBDClient *client, 
> > NBDExportMetaContexts *meta,
> > +                                const char *ns, uint32_t len, Error 
> > **errp)
> >   {
> > -    bool dirty_bitmap = false;
> > -    size_t dirty_bitmap_len = strlen("dirty-bitmap:");
> > -    int ret;
> > -
> >       if (!meta->exp->export_bitmap) {
> >           trace_nbd_negotiate_meta_query_skip("no dirty-bitmap 
> > exported");
> > -        return nbd_opt_skip(client, len, errp);
> > -    }
> > -
> > -    if (len == 0) {
> > +    } else if (len == 0) {
>
> s/0/5/ ?

Oh, good catch.  Using 0 is an unintended logic change, but none of our 
iotests caught it, so we're also missing test coverage.

I'm working on adding 'nbd_opt_list_meta_context()' to libnbd, which 
will add testsuite coverage of this.


> > +    } else if (!g_str_has_prefix(ns + 5, "dirty-bitmap:")) {
> >           trace_nbd_negotiate_meta_query_skip("not dirty-bitmap:");
> > -        return nbd_opt_skip(client, len, errp);
> > +    } else {
> > +        trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
> > +        nbd_meta_empty_or_pattern(client, 
> > meta->exp->export_bitmap_context,
> > +                                  ns, len, &meta->bitmap, errp);
>
> hmm. _empty_ is impossible here, as ns includes "qemu:dirty-bitmap"..

Ultimately, we want something like:

if starts with "base:":
  if nothing else:
    if list mode:
      return "base:allocation"
    else:
      return empty list
  else if "base:allocation":
    return "base:allocation"
  else
    return empty list
else if starts with "qemu:":
  if nothing else:
    if list mode:
      return all possible sub-qemu contexts
    else:
      return empty list
  else if starts with "qemu:dirty-bitmap:":
    if nothing else:
      if list mode:
        return all possible dirty-bitmap contexts (right now, just one,
            but I want to allow an array of bitmaps in the future)
      else:
        return empty list
    else if exact match:
      return that bitmap
    else:
      return empty list
  else if "qemu:allocation-depth" (from next patch):
    return "allocation-depth"
  else:
    return empty list
else:
  return empty list

Maybe some function renames will help.

> >       if (ret <= 0) {
> >           return ret;
> >       }
> > +    ns[len] = '\0';
> > +    if (strlen(ns) != len) {
> > +        return nbd_opt_invalid(client, errp,
> > +                               "Embedded NUL in query for option %s",
> > +                               nbd_opt_lookup(client->opt));
> > +    }
>
> Hmm, that's a new good check. Intersting, is it specified in NBD 
> protocol, that
> NUL shouldn't be in a string.. Probably it can be a separate patch, with
> new nbd_opt_string_read, which will check this thing. But I'm OK with it 
> as is
> in this patch.

I'll separate it.

> To avoid all this pointer arithmetic, what about something like this (I 
> didn't check, just an idea):

I'll see what parts of that I can use in v2.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org