Libguestfs - Feb 2020 - Re: [nbdkit PATCH] eval: Allow user override of 'missing'

A comment in the code mentioned something that didn't actually work,
but which can be useful for user-directed logging of what other
callbacks they might want to implement.

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

I haven't pushed this one, becuase I'm not sure if we want it; but it
was easy enough to whip together after an IRC question earlier today.

 plugins/eval/eval.c                 | 16 +++++++++++++---
 plugins/eval/nbdkit-eval-plugin.pod |  6 ++++++
 tests/test-eval.sh                  |  2 ++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c
index 8f1eb6c..094cac5 100644
--- a/plugins/eval/eval.c
+++ b/plugins/eval/eval.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -169,6 +169,12 @@ create_script (const char *method, const char *value)
     return NULL;
   }

+  /* Special case for user override of missing */
+  if (missing && strcmp (script, missing) == 0 && unlink
(script) == -1) {
+    nbdkit_error ("unlink: %m");
+    return NULL;
+  }
+
   fp = fopen (script, "w");
   if (fp == NULL) {
     nbdkit_error ("fopen: %s: %m", script);
@@ -216,7 +222,7 @@ eval_load (void)

   /* To make things easier, create a "missing" script which always
    * exits with code 2.  If a method is missing we call this script
-   * instead.  It could even be overridden by the user.
+   * instead.  It can even be overridden by the user.
    */
   missing = create_script ("missing", "exit 2\n");
   if (!missing)
@@ -255,11 +261,15 @@ static int
 add_method (const char *key, const char *value)
 {
   char *script;
+  char *tmp = missing; /* Needed to allow user override of missing */

-  if (get_script (key) != missing) {
+  missing = NULL;
+  if (get_script (key) != NULL) {
+    missing = tmp;
     nbdkit_error ("method %s defined more than once on the command
line", key);
     return -1;
   }
+  missing = tmp;

   /* Do a bit of checking to make sure the key isn't malicious.  This
    * duplicates work already done by nbdkit, but better safe than
diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
index 88c1488..cbb4133 100644
--- a/plugins/eval/nbdkit-eval-plugin.pod
+++ b/plugins/eval/nbdkit-eval-plugin.pod
@@ -140,6 +140,12 @@ no longer see that key.

 All of these parameters are optional.

+=item B<missing=>SCRIPT
+
+The parameter C<missing> defines a script that will be called in place
+of any other callback not explicitly provided.  If omitted, this
+defaults to the script "exit 2".
+
 =back

 =head1 ENVIRONMENT VARIABLES
diff --git a/tests/test-eval.sh b/tests/test-eval.sh
index 206c680..4557b02 100755
--- a/tests/test-eval.sh
+++ b/tests/test-eval.sh
@@ -42,7 +42,9 @@ cleanup_fn rm -f $files
 nbdkit -U - eval \
        get_size='echo 64M' \
        pread='dd if=/dev/zero count=$3 iflag=count_bytes' \
+       missing='echo "in missing: $@" >> eval.out; exit
2' \
        --run 'qemu-img info $nbd' > eval.out

 cat eval.out
 grep '67108864 bytes' eval.out
+grep 'in missing' eval.out
-- 
2.24.1

On 2/10/20 1:44 PM, Eric Blake wrote:
> A comment in the code mentioned something that didn't actually work,
> but which can be useful for user-directed logging of what other
> callbacks they might want to implement.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> I haven't pushed this one, becuase I'm not sure if we want it; but
it
> was easy enough to whip together after an IRC question earlier today.
> 
>   plugins/eval/eval.c                 | 16 +++++++++++++---
>   plugins/eval/nbdkit-eval-plugin.pod |  6 ++++++
>   tests/test-eval.sh                  |  2 ++
>   3 files changed, 21 insertions(+), 3 deletions(-)
Missing one hunk:

diff --git i/plugins/eval/eval.c w/plugins/eval/eval.c
index 31b62bb..094cac5 100644
--- i/plugins/eval/eval.c
+++ w/plugins/eval/eval.c
@@ -72,6 +72,7 @@ static const char *known_methods[] = {
    "flush",
    "get_size",
    "is_rotational",
+  "missing",
    "open",
    "pread",
    "pwrite",


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

On Mon, Feb 10, 2020 at 01:44:14PM -0600, Eric Blake
wrote:
> A comment in the code mentioned something that didn't actually work,
> but which can be useful for user-directed logging of what other
> callbacks they might want to implement.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> I haven't pushed this one, becuase I'm not sure if we want it; but
it
> was easy enough to whip together after an IRC question earlier today.
> 
>  plugins/eval/eval.c                 | 16 +++++++++++++---
>  plugins/eval/nbdkit-eval-plugin.pod |  6 ++++++
>  tests/test-eval.sh                  |  2 ++
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c
> index 8f1eb6c..094cac5 100644
> --- a/plugins/eval/eval.c
> +++ b/plugins/eval/eval.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018-2019 Red Hat Inc.
> + * Copyright (C) 2018-2020 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -169,6 +169,12 @@ create_script (const char *method, const char *value)
>      return NULL;
>    }
> 
> +  /* Special case for user override of missing */
> +  if (missing && strcmp (script, missing) == 0 && unlink
(script) == -1) {
> +    nbdkit_error ("unlink: %m");
> +    return NULL;
> +  }
> +
>    fp = fopen (script, "w");
>    if (fp == NULL) {
>      nbdkit_error ("fopen: %s: %m", script);
> @@ -216,7 +222,7 @@ eval_load (void)
> 
>    /* To make things easier, create a "missing" script which
always
>     * exits with code 2.  If a method is missing we call this script
> -   * instead.  It could even be overridden by the user.
> +   * instead.  It can even be overridden by the user.
>     */
>    missing = create_script ("missing", "exit 2\n");
>    if (!missing)
> @@ -255,11 +261,15 @@ static int
>  add_method (const char *key, const char *value)
>  {
>    char *script;
> +  char *tmp = missing; /* Needed to allow user override of missing */
> 
> -  if (get_script (key) != missing) {
> +  missing = NULL;
> +  if (get_script (key) != NULL) {
> +    missing = tmp;
>      nbdkit_error ("method %s defined more than once on the command
line", key);
>      return -1;
>    }
> +  missing = tmp;
This leaks the missing global?

But yes the idea is fine, so ACK.

Rich.
>    /* Do a bit of checking to make sure the key isn't malicious.  This
>     * duplicates work already done by nbdkit, but better safe than
> diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
> index 88c1488..cbb4133 100644
> --- a/plugins/eval/nbdkit-eval-plugin.pod
> +++ b/plugins/eval/nbdkit-eval-plugin.pod
> @@ -140,6 +140,12 @@ no longer see that key.
> 
>  All of these parameters are optional.
> 
> +=item B<missing=>SCRIPT
> +
> +The parameter C<missing> defines a script that will be called in
place
> +of any other callback not explicitly provided.  If omitted, this
> +defaults to the script "exit 2".
> +
>  =back
> 
>  =head1 ENVIRONMENT VARIABLES
> diff --git a/tests/test-eval.sh b/tests/test-eval.sh
> index 206c680..4557b02 100755
> --- a/tests/test-eval.sh
> +++ b/tests/test-eval.sh
> @@ -42,7 +42,9 @@ cleanup_fn rm -f $files
>  nbdkit -U - eval \
>         get_size='echo 64M' \
>         pread='dd if=/dev/zero count=$3 iflag=count_bytes' \
> +       missing='echo "in missing: $@" >> eval.out;
exit 2' \
>         --run 'qemu-img info $nbd' > eval.out
> 
>  cat eval.out
>  grep '67108864 bytes' eval.out
> +grep 'in missing' eval.out
> -- 
> 2.24.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 2/10/20 3:10 PM, Richard W.M. Jones wrote:
> On Mon, Feb 10, 2020 at 01:44:14PM -0600, Eric Blake wrote:
>> A comment in the code mentioned something that didn't actually
work,
>> but which can be useful for user-directed logging of what other
>> callbacks they might want to implement.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> I haven't pushed this one, becuase I'm not sure if we want it;
but it
>> was easy enough to whip together after an IRC question earlier today.
>>
>> @@ -255,11 +261,15 @@ static int
>>   add_method (const char *key, const char *value)
>>   {
>>     char *script;
>> +  char *tmp = missing; /* Needed to allow user override of missing */
>>
>> -  if (get_script (key) != missing) {
>> +  missing = NULL;
>> +  if (get_script (key) != NULL) {
>> +    missing = tmp;
>>       nbdkit_error ("method %s defined more than once on the
command line", key);
>>       return -1;
>>     }
>> +  missing = tmp;
> 
> This leaks the missing global?
Surprisingly not.  We end up storing two malloc'd strings pointing to 
the same script name, one in 'missing' (which gets freed independently),
and one in 'method_scripts[index_of_missing]' (which gets freed in our 
pass over method_scripts).  But yes, I'll make sure to double-check a 
valgrind run before committing.
> 
> But yes the idea is fine, so ACK.
> 
> Rich.
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org