[1/4] nbd/server: refactor nbd_negotiate_meta_query for several namespaces

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 17 deletions(-)

[adding NBD list]

On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 17 deletions(-)

> +struct {
> +    const char *ns;
> +    int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, Error **);
> +} meta_namespace_handlers[] = {
> +    /* namespaces should go in non-decreasing order by name length */
> +    {.ns = "base:", .func = nbd_meta_base_query},
> +};
> +

> @@ -787,9 +793,12 @@ static int nbd_negotiate_meta_query(NBDClient *client,
>                                       NBDExportMetaContexts *meta, Error **errp)
>   {
>       int ret;
> -    char query[sizeof("base:") - 1];
> -    size_t baselen = strlen("base:");
> +    int i;
>       uint32_t len;
> +    int bytes_done = 0;
> +    char *query;
> +    int nb_ns = sizeof(meta_namespace_handlers) /
> +                sizeof(meta_namespace_handlers[0]);

Use the ARRAY_SIZE() macro here.

> +    query = g_malloc(strlen(meta_namespace_handlers[nb_ns - 1].ns));

So this sizes a buffer according to the largest namespace we expect to 
handle,...

>   
> -    len -= baselen;
> -    ret = nbd_opt_read(client, query, baselen, errp);
> -    if (ret <= 0) {
> -        return ret;
> -    }
> -    if (strncmp(query, "base:", baselen) != 0) {
> -        return nbd_opt_skip(client, len, errp);
> +    for (i = 0; i < nb_ns; i++) {
> +        const char *ns = meta_namespace_handlers[i].ns;
> +        int ns_len = strlen(ns);
> +        int diff_len = strlen(ns) - bytes_done;
> +
> +        assert(diff_len >= 0);
> +
> +        if (diff_len > 0) {
> +            if (len < diff_len) {
> +                ret = nbd_opt_skip(client, len, errp);
> +                goto out;
> +            }
> +
> +            len -= diff_len;
> +            ret = nbd_opt_read(client, query + bytes_done, diff_len, errp);
> +            if (ret <= 0) {
> +                goto out;
> +            }
> +        }
> +
> +        if (!strncmp(query, ns, ns_len)) {
> +            ret = meta_namespace_handlers[i].func(client, meta, len, errp);
> +            goto out;
> +        }


...then you do multiple iterative reads as you step through the list. 
You know, if you encounter a ':' at any point in the iterative reads, 
you don't have to continue through the rest of the handlers (the query 
belongs to a short-named namespace we didn't recognize).

Is it any smarter to just blindly do a single read of MIN(querylen, 
maxlen), then strchr() for ':' (if not found, it's not a namespace we 
recognize), and then look up if the name matches, at which point we then 
read the rest of the query and refactor the namespace handler to be 
passed the already-parsed leafname, rather than having to parse the 
leafname off the wire in the handler?

I'm STILL wondering if the NBD spec should specify namespace and 
leafname as separate fields rather than requiring the server to parse 
for ':'.  We have only a couple of weeks before the qemu 2.12 release 
cements in place an implementation of the BLOCK_STATUS extension.  And 
we've already discussed that if we make a change, we have to consider 
using a different constant for NBD_OPT_SET_META_CONTEXT to play nicely 
with the existing Virtuozzo implementation that currently matches what 
is slated to land in qemu 2.12 if no further qemu patches are submitted. 
  Is it worth me proposing a doc change to demonstrate what the 
difference would look like, along with corresponding qemu changes to 
match, to decide if including it in 2.12 is worth it?

On Wed, Mar 21, 2018 at 09:56:36AM -0500, Eric Blake wrote:
> no further qemu patches are submitted.  Is it worth me proposing a doc
> change to demonstrate what the difference would look like, along with
> corresponding qemu changes to match, to decide if including it in 2.12 is
> worth it?

If you're willing to spend the effort, I won't stop you :-) but I think the
pain of parsing a string once (during negotiation) is not that bad, and
I'm not too bothered about it.

Put otherwise, I'm not convinced that the downside (which I agree
exists) outweighs the downside of having to introduce yet another
command, or add some ugliness for backwards compat reasons.

But hey, go ahead, prove me wrong ;-)

21.03.2018 17:56, Eric Blake wrote:
> [adding NBD list]
>
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 60 
>> +++++++++++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 43 insertions(+), 17 deletions(-)
>
>> +struct {
>> +    const char *ns;
>> +    int (*func)(NBDClient *, NBDExportMetaContexts *, uint32_t, 
>> Error **);
>> +} meta_namespace_handlers[] = {
>> +    /* namespaces should go in non-decreasing order by name length */
>> +    {.ns = "base:", .func = nbd_meta_base_query},
>> +};
>> +
>
>> @@ -787,9 +793,12 @@ static int nbd_negotiate_meta_query(NBDClient 
>> *client,
>>                                       NBDExportMetaContexts *meta, 
>> Error **errp)
>>   {
>>       int ret;
>> -    char query[sizeof("base:") - 1];
>> -    size_t baselen = strlen("base:");
>> +    int i;
>>       uint32_t len;
>> +    int bytes_done = 0;
>> +    char *query;
>> +    int nb_ns = sizeof(meta_namespace_handlers) /
>> +                sizeof(meta_namespace_handlers[0]);
>
> Use the ARRAY_SIZE() macro here.
>
>> +    query = g_malloc(strlen(meta_namespace_handlers[nb_ns - 1].ns));
>
> So this sizes a buffer according to the largest namespace we expect to 
> handle,...
>
>>   -    len -= baselen;
>> -    ret = nbd_opt_read(client, query, baselen, errp);
>> -    if (ret <= 0) {
>> -        return ret;
>> -    }
>> -    if (strncmp(query, "base:", baselen) != 0) {
>> -        return nbd_opt_skip(client, len, errp);
>> +    for (i = 0; i < nb_ns; i++) {
>> +        const char *ns = meta_namespace_handlers[i].ns;
>> +        int ns_len = strlen(ns);
>> +        int diff_len = strlen(ns) - bytes_done;
>> +
>> +        assert(diff_len >= 0);
>> +
>> +        if (diff_len > 0) {
>> +            if (len < diff_len) {
>> +                ret = nbd_opt_skip(client, len, errp);
>> +                goto out;
>> +            }
>> +
>> +            len -= diff_len;
>> +            ret = nbd_opt_read(client, query + bytes_done, diff_len, 
>> errp);
>> +            if (ret <= 0) {
>> +                goto out;
>> +            }
>> +        }
>> +
>> +        if (!strncmp(query, ns, ns_len)) {
>> +            ret = meta_namespace_handlers[i].func(client, meta, len, 
>> errp);
>> +            goto out;
>> +        }
>
>
> ...then you do multiple iterative reads as you step through the list. 
> You know, if you encounter a ':' at any point in the iterative reads, 
> you don't have to continue through the rest of the handlers (the query 
> belongs to a short-named namespace we didn't recognize).
>
> Is it any smarter to just blindly do a single read of MIN(querylen, 
> maxlen), then strchr() for ':' (if not found, it's not a namespace we 
> recognize), and then look up if the name matches, at which point we 
> then read the rest of the query and refactor the namespace handler to 
> be passed the already-parsed leafname, rather than having to parse the 
> leafname off the wire in the handler?

but what if we get base:very-long-garbage? Only namespace handler knows, 
should we read leafname to the memory or just drop. So, in your case, 
we'll have to refactor it to handle partly read query..
for now, we can do it as simple as:

1. read first letter.
2. if first_letter == 'b' and len == strlen(base:alloction): read, 
check, set meta.base_allocation if match
3. if first_letter == 'q' and len == strlen(qemu-drity-bitmap:) + 
strlen(exp.export_bitmap_name): read, check, set meta.dirty_bitmap if match

and forget about generic code, until we really need it

>
> I'm STILL wondering if the NBD spec should specify namespace and 
> leafname as separate fields rather than requiring the server to parse 
> for ':'.  We have only a couple of weeks before the qemu 2.12 release 
> cements in place an implementation of the BLOCK_STATUS extension.  And 
> we've already discussed that if we make a change, we have to consider 
> using a different constant for NBD_OPT_SET_META_CONTEXT to play nicely 
> with the existing Virtuozzo implementation that currently matches what 
> is slated to land in qemu 2.12 if no further qemu patches are 
> submitted.  Is it worth me proposing a doc change to demonstrate what 
> the difference would look like, along with corresponding qemu changes 
> to match, to decide if including it in 2.12 is worth it?
>

Personally, I'd prefer to avoid it.