Add nbd_meta_pattern() and nbd_meta_empty_or_pattern() helpers for
metadata query parsing. nbd_meta_pattern() will be reused for "qemu"
namespace in following patches.

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

diff --git a/nbd/server.c b/nbd/server.c
index 567561a77e..2d762d7289 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -733,52 +733,83 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
     return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0;
 }
 
-/* nbd_meta_base_query
- *
- * Handle query to 'base' namespace. For now, only base:allocation context is
- * available in it.  'len' is the amount of text remaining to be read from
- * the current name, after the 'base:' portion has been stripped.
+/* 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 parsed "base:allocation"
- * namespace. It only means that there are no errors.*/
-static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
-                               uint32_t len, Error **errp)
+ * Note: return code = 1 doesn't mean that we've read exactly @pattern
+ * It only means that there are no errors. */
+static int nbd_meta_pattern(NBDClient *client, const char *pattern, bool *match,
+                            Error **errp)
 {
     int ret;
-    char query[sizeof("allocation") - 1];
-    size_t alen = strlen("allocation");
+    char *query;
+    int len = strlen(pattern);
 
-    if (len == 0) {
-        if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
-            meta->base_allocation = true;
-        }
-        trace_nbd_negotiate_meta_query_parse("base:");
-        return 1;
-    }
-
-    if (len != alen) {
-        trace_nbd_negotiate_meta_query_skip("not base:allocation");
-        return nbd_opt_skip(client, len, errp);
-    }
+    assert(len);
 
+    query = g_malloc(len);
     ret = nbd_opt_read(client, query, len, errp);
     if (ret <= 0) {
+        g_free(query);
         return ret;
     }
 
-    if (strncmp(query, "allocation", alen) == 0) {
-        trace_nbd_negotiate_meta_query_parse("base:allocation");
-        meta->base_allocation = true;
+    if (strncmp(query, pattern, len) == 0) {
+        trace_nbd_negotiate_meta_query_parse(pattern);
+        *match = true;
     } else {
-        trace_nbd_negotiate_meta_query_skip("not base:allocation");
+        trace_nbd_negotiate_meta_query_skip(pattern);
     }
+    g_free(query);
 
     return 1;
 }
 
+/* Read @len bytes, and set @match to true if they match @pattern, or if @len
+ * is 0 and the client is performing _LIST_. @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. */
+static int nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
+                                     uint32_t len, bool *match, Error **errp)
+{
+    if (len == 0) {
+        if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
+            *match = true;
+        }
+        trace_nbd_negotiate_meta_query_parse("empty");
+        return 1;
+    }
+
+    if (len != strlen(pattern)) {
+        trace_nbd_negotiate_meta_query_skip("different lengths");
+        return nbd_opt_skip(client, len, errp);
+    }
+
+    return nbd_meta_pattern(client, pattern, match, errp);
+}
+
+/* nbd_meta_base_query
+ *
+ * Handle query to 'base' namespace. For now, only base:allocation context is
+ * available in it.  'len' is the amount of text remaining to be read from
+ * the current name, after the 'base:' portion has been stripped.
+ *
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
+                               uint32_t len, Error **errp)
+{
+    return nbd_meta_empty_or_pattern(client, "allocation", len,
+                                     &meta->base_allocation, errp);
+}
+
 /* nbd_negotiate_meta_query
  *
  * Parse namespace name and call corresponding function to parse body of the
@@ -822,6 +853,7 @@ static int nbd_negotiate_meta_query(NBDClient *client,
         return nbd_opt_skip(client, len, errp);
     }
 
+    trace_nbd_negotiate_meta_query_parse("base:");
     return nbd_meta_base_query(client, meta, len, errp);
 }
 
-- 
2.11.1

On 06/09/2018 10:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add nbd_meta_pattern() and nbd_meta_empty_or_pattern() helpers for
> metadata query parsing. nbd_meta_pattern() will be reused for "qemu"

s/for/for the/

> namespace in following patches.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 86 +++++++++++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 59 insertions(+), 27 deletions(-)

Feels like growth, even though the goal of refactoring is reuse; but the 
reuse comes later so I'm okay with it.

> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 567561a77e..2d762d7289 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -733,52 +733,83 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
>       return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? -EIO : 0;
>   }
>   
> -/* nbd_meta_base_query
> - *
> - * Handle query to 'base' namespace. For now, only base:allocation context is

[1]...

> - * available in it.  'len' is the amount of text remaining to be read from
> - * the current name, after the 'base:' portion has been stripped.
> +/* 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 parsed "base:allocation"
> - * namespace. It only means that there are no errors.*/
> -static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
> -                               uint32_t len, Error **errp)
> + * Note: return code = 1 doesn't mean that we've read exactly @pattern
> + * It only means that there are no errors. */

Comment tail on its own line (now that we've got a patch pending for 
HACKING to document that, I'll start abiding by it...)

> +static int nbd_meta_pattern(NBDClient *client, const char *pattern, bool *match,
> +                            Error **errp)
>   {
>       int ret;
> -    char query[sizeof("allocation") - 1];
> -    size_t alen = strlen("allocation");
> +    char *query;
> +    int len = strlen(pattern);

size_t is better than len for strlen() results.

>   
> -    if (len == 0) {
> -        if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
> -            meta->base_allocation = true;
> -        }
> -        trace_nbd_negotiate_meta_query_parse("base:");
> -        return 1;
> -    }
> -
> -    if (len != alen) {
> -        trace_nbd_negotiate_meta_query_skip("not base:allocation");
> -        return nbd_opt_skip(client, len, errp);
> -    }
> +    assert(len);
>   
> +    query = g_malloc(len);

At first, I wondered if we could just use a pre-allocated stack buffer 
larger than any string we ever anticipate.  But thinking about it, your 
dirty bitmap exports expose a name under user control, which means a 
user could (spitefully) pick a name longer than our buffer (well, up to 
the 4k name limit imposed by the NBD protocol).  So I can live with the 
malloc.

>       ret = nbd_opt_read(client, query, len, errp);
>       if (ret <= 0) {
> +        g_free(query);
>           return ret;
>       }
>   
> -    if (strncmp(query, "allocation", alen) == 0) {
> -        trace_nbd_negotiate_meta_query_parse("base:allocation");
> -        meta->base_allocation = true;
> +    if (strncmp(query, pattern, len) == 0) {
> +        trace_nbd_negotiate_meta_query_parse(pattern);
> +        *match = true;
>       } else {
> -        trace_nbd_negotiate_meta_query_skip("not base:allocation");
> +        trace_nbd_negotiate_meta_query_skip(pattern);

Would this one read better as "not %s", pattern?

>       }
> +    g_free(query);
>   
>       return 1;
>   }
>   
> +/* Read @len bytes, and set @match to true if they match @pattern, or if @len
> + * is 0 and the client is performing _LIST_. @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. */

More comment formatting.

> +static int nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
> +                                     uint32_t len, bool *match, Error **errp)
> +{
> +    if (len == 0) {
> +        if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
> +            *match = true;
> +        }
> +        trace_nbd_negotiate_meta_query_parse("empty");
> +        return 1;
> +    }
> +
> +    if (len != strlen(pattern)) {
> +        trace_nbd_negotiate_meta_query_skip("different lengths");
> +        return nbd_opt_skip(client, len, errp);
> +    }
> +
> +    return nbd_meta_pattern(client, pattern, match, errp);
> +}
> +
> +/* nbd_meta_base_query
> + *
> + * Handle query to 'base' namespace. For now, only base:allocation context is

Pre-existing (see [1]), but reads better as "Handle queries to the 
'base' namespace"

> + * available in it.  'len' is the amount of text remaining to be read from
> + * the current name, after the 'base:' portion has been stripped.
> + *
> + * Return -errno on I/O error, 0 if option was completely handled by
> + * sending a reply about inconsistent lengths, or 1 on success. */
> +static int nbd_meta_base_query(NBDClient *client, NBDExportMetaContexts *meta,
> +                               uint32_t len, Error **errp)
> +{
> +    return nbd_meta_empty_or_pattern(client, "allocation", len,
> +                                     &meta->base_allocation, errp);
> +}
> +
>   /* nbd_negotiate_meta_query
>    *
>    * Parse namespace name and call corresponding function to parse body of the
> @@ -822,6 +853,7 @@ static int nbd_negotiate_meta_query(NBDClient *client,
>           return nbd_opt_skip(client, len, errp);
>       }
>   
> +    trace_nbd_negotiate_meta_query_parse("base:");
>       return nbd_meta_base_query(client, meta, len, errp);
>   }
>   
> 

My findings were trivial; the code refactoring itself looks sane.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

19.06.2018 23:24, Eric Blake wrote:
> On 06/09/2018 10:17 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Add nbd_meta_pattern() and nbd_meta_empty_or_pattern() helpers for
>> metadata query parsing. nbd_meta_pattern() will be reused for "qemu"
>
> s/for/for the/
>
>> namespace in following patches.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 86 
>> +++++++++++++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 59 insertions(+), 27 deletions(-)
>
> Feels like growth, even though the goal of refactoring is reuse; but 
> the reuse comes later so I'm okay with it.
>
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 567561a77e..2d762d7289 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -733,52 +733,83 @@ static int 
>> nbd_negotiate_send_meta_context(NBDClient *client,
>>       return qio_channel_writev_all(client->ioc, iov, 2, errp) < 0 ? 
>> -EIO : 0;
>>   }
>>   -/* nbd_meta_base_query
>> - *
>> - * Handle query to 'base' namespace. For now, only base:allocation 
>> context is
>
> [1]...
>
>> - * available in it.  'len' is the amount of text remaining to be 
>> read from
>> - * the current name, after the 'base:' portion has been stripped.
>> +/* 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 parsed 
>> "base:allocation"
>> - * namespace. It only means that there are no errors.*/
>> -static int nbd_meta_base_query(NBDClient *client, 
>> NBDExportMetaContexts *meta,
>> -                               uint32_t len, Error **errp)
>> + * Note: return code = 1 doesn't mean that we've read exactly @pattern
>> + * It only means that there are no errors. */
>
> Comment tail on its own line (now that we've got a patch pending for 
> HACKING to document that, I'll start abiding by it...)
>
>> +static int nbd_meta_pattern(NBDClient *client, const char *pattern, 
>> bool *match,
>> +                            Error **errp)
>>   {
>>       int ret;
>> -    char query[sizeof("allocation") - 1];
>> -    size_t alen = strlen("allocation");
>> +    char *query;
>> +    int len = strlen(pattern);
>
> size_t is better than len for strlen() results.
>
>>   -    if (len == 0) {
>> -        if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
>> -            meta->base_allocation = true;
>> -        }
>> -        trace_nbd_negotiate_meta_query_parse("base:");
>> -        return 1;
>> -    }
>> -
>> -    if (len != alen) {
>> -        trace_nbd_negotiate_meta_query_skip("not base:allocation");
>> -        return nbd_opt_skip(client, len, errp);
>> -    }
>> +    assert(len);
>>   +    query = g_malloc(len);
>
> At first, I wondered if we could just use a pre-allocated stack buffer 
> larger than any string we ever anticipate.  But thinking about it, 
> your dirty bitmap exports expose a name under user control, which 
> means a user could (spitefully) pick a name longer than our buffer 
> (well, up to the 4k name limit imposed by the NBD protocol).  So I can 
> live with the malloc.
>
>>       ret = nbd_opt_read(client, query, len, errp);
>>       if (ret <= 0) {
>> +        g_free(query);
>>           return ret;
>>       }
>>   -    if (strncmp(query, "allocation", alen) == 0) {
>> - trace_nbd_negotiate_meta_query_parse("base:allocation");
>> -        meta->base_allocation = true;
>> +    if (strncmp(query, pattern, len) == 0) {
>> +        trace_nbd_negotiate_meta_query_parse(pattern);
>> +        *match = true;
>>       } else {
>> -        trace_nbd_negotiate_meta_query_skip("not base:allocation");
>> +        trace_nbd_negotiate_meta_query_skip(pattern);
>
> Would this one read better as "not %s", pattern?

may be, but how? introduce malloced string variable to sprintf?

>
>>       }
>> +    g_free(query);
>>         return 1;
>>   }
>>   +/* Read @len bytes, and set @match to true if they match @pattern, 
>> or if @len
>> + * is 0 and the client is performing _LIST_. @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. */
>
> More comment formatting.

hm, we need something in checkpatch for it

>
>> +static int nbd_meta_empty_or_pattern(NBDClient *client, const char 
>> *pattern,
>> +                                     uint32_t len, bool *match, 
>> Error **errp)
>> +{
>> +    if (len == 0) {
>> +        if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
>> +            *match = true;
>> +        }
>> +        trace_nbd_negotiate_meta_query_parse("empty");
>> +        return 1;
>> +    }
>> +
>> +    if (len != strlen(pattern)) {
>> +        trace_nbd_negotiate_meta_query_skip("different lengths");
>> +        return nbd_opt_skip(client, len, errp);
>> +    }
>> +
>> +    return nbd_meta_pattern(client, pattern, match, errp);
>> +}
>> +
>> +/* nbd_meta_base_query
>> + *
>> + * Handle query to 'base' namespace. For now, only base:allocation 
>> context is
>
> Pre-existing (see [1]), but reads better as "Handle queries to the 
> 'base' namespace"
>
>> + * available in it.  'len' is the amount of text remaining to be 
>> read from
>> + * the current name, after the 'base:' portion has been stripped.
>> + *
>> + * Return -errno on I/O error, 0 if option was completely handled by
>> + * sending a reply about inconsistent lengths, or 1 on success. */
>> +static int nbd_meta_base_query(NBDClient *client, 
>> NBDExportMetaContexts *meta,
>> +                               uint32_t len, Error **errp)
>> +{
>> +    return nbd_meta_empty_or_pattern(client, "allocation", len,
>> + &meta->base_allocation, errp);
>> +}
>> +
>>   /* nbd_negotiate_meta_query
>>    *
>>    * Parse namespace name and call corresponding function to parse 
>> body of the
>> @@ -822,6 +853,7 @@ static int nbd_negotiate_meta_query(NBDClient 
>> *client,
>>           return nbd_opt_skip(client, len, errp);
>>       }
>>   +    trace_nbd_negotiate_meta_query_parse("base:");
>>       return nbd_meta_base_query(client, meta, len, errp);
>>   }
>>
>
> My findings were trivial; the code refactoring itself looks sane.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>


-- 
Best regards,
Vladimir