Libguestfs - Jul 2022 - [nbdkit] ARRAY_SIZE [was: vddk: Demote another "phone home" error message to debug]

On 07/07/22 11:02, Richard W.M. Jones wrote:
> Reported-by: Ming Xie
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2104720
> ---
>  plugins/vddk/vddk.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index dbd3fdbe..1ed9fc53 100644
> --- a/plugins/vddk/vddk.c
> +++ b/plugins/vddk/vddk.c
> @@ -495,11 +495,25 @@ debug_function (const char *fs, va_list args)
>    nbdkit_debug ("%s", str);
>  }
>  
> +/* VDDK 7 added some useless error messages about their "phone
home"
> + * system called CEIP which only panics users.  Demote these to debug
> + * statements below.
> + *
> + * https://bugzilla.redhat.com/show_bug.cgi?id=1834267
> + * https://bugzilla.redhat.com/show_bug.cgi?id=2083617
> + * https://bugzilla.redhat.com/show_bug.cgi?id=2104720
> + */
> +static const char *demoted_errors[] = {
We could constify the "demoted_errors" array itself too, not just the
pointed-to strings.
> +  "Get CEIP status failed",
> +  "VDDK_PhoneHome:",
> +};
> +
>  /* Turn error messages from the library into nbdkit_error. */
>  static void
>  error_function (const char *fs, va_list args)
>  {
>    CLEANUP_FREE char *str = NULL;
> +  size_t i;
>  
>    /* If the thread-local error_suppression flag is non-zero then we
>     * will suppress error messages from VDDK in this thread.
> @@ -513,17 +527,12 @@ error_function (const char *fs, va_list args)
>  
>    trim (str);
>  
> -  /* VDDK 7 added some useless error messages about their "phone
home"
> -   * system called CEIP which only panics users.  Demote to a debug
> -   * statement.
> -   * https://bugzilla.redhat.com/show_bug.cgi?id=1834267
> -   * https://bugzilla.redhat.com/show_bug.cgi?id=2083617
> -   */
> -  if (strstr (str, "Get CEIP status failed") != NULL ||
> -      strstr (str, "VDDK_PhoneHome: Unable to load configuration
"
> -                   "options from ") != NULL) {
> -    nbdkit_debug ("%s", str);
> -    return;
> +  /* See comment above about demoted errors. */
> +  for (i = 0; i < sizeof demoted_errors / sizeof demoted_errors[0];
++i) {
> +    if (strstr (str, demoted_errors[i]) != NULL) {
> +      nbdkit_debug ("%s", str);
> +      return;
> +    }
>    }
>  
>    nbdkit_error ("%s", str);
> 
Humble -- yet annoying! -- request:

$ git grep -E 'sizeof ([a-zA-Z0-9_]+) / sizeof \1\[0\]'

common/bitmap/test-bitmap.c:  for (j = 0; j < sizeof blks / sizeof blks[0];
++j) {
common/bitmap/test-bitmap.c:    for (i = 0; i < sizeof blksizes / sizeof
blksizes[0]; ++i)
common/include/test-random.c:#define nr_tests (sizeof tests / sizeof tests[0])
common/utils/test-quotes.c:  for (i = 0; i < sizeof tests / sizeof tests[0];
i++) {
plugins/ocaml/plugin.c:  rv = caml_callbackN_exn (pread_fn, sizeof args / sizeof
args[0], args);
plugins/ocaml/plugin.c:  rv = caml_callbackN_exn (pwrite_fn, sizeof args /
sizeof args[0], args);
plugins/ocaml/plugin.c:  rv = caml_callbackN_exn (trim_fn, sizeof args / sizeof
args[0], args);
plugins/ocaml/plugin.c:  rv = caml_callbackN_exn (zero_fn, sizeof args / sizeof
args[0], args);
plugins/ocaml/plugin.c:  rv = caml_callbackN_exn (extents_fn, sizeof args /
sizeof args[0], args);
plugins/ocaml/plugin.c:  rv = caml_callbackN_exn (cache_fn, sizeof args / sizeof
args[0], args);
plugins/ruby/ruby.c:    code = ruby_options (sizeof options / sizeof options[0],
plugins/ssh/ssh.c:  const size_t nr_levels = sizeof levels / sizeof levels[0];
plugins/torrent/torrent.cpp:static const size_t nr_settings = sizeof settings /
sizeof settings[0];
server/fuzzer.c:  const int argc = sizeof argv / sizeof argv[0] - 1;
server/public.c:  switch (ppoll (fds, sizeof fds / sizeof fds[0], &ts,
&all)) {
server/test-public.c:  for (i = 0; i < sizeof pairs / sizeof pairs[0]; i++) {

I think it's time for us to introduce the ARRAY_SIZE macro.

Can you do that first perhaps, rebase the existing code, and then add this new
patch? :)

However you decide:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks!
Laszlo

On Thu, Jul 07, 2022 at 12:58:53PM +0200, Laszlo Ersek
wrote:
> I think it's time for us to introduce the ARRAY_SIZE macro.
Yeah I thought about that while I was writing the patch :-)

I'll do as you suggest, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

On Thu, Jul 07, 2022 at 12:58:53PM +0200, Laszlo Ersek
wrote:
> On 07/07/22 11:02, Richard W.M. Jones wrote:
> > Reported-by: Ming Xie
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2104720
> > ---
> I think it's time for us to introduce the ARRAY_SIZE macro.
> 
> Can you do that first perhaps, rebase the existing code, and then add this
new patch? :)
I see that is now done as part of 0fa23df5c.  While
include/array-size.h looks okay, I personally have a tough time with
include/compiler-macros.h BUILD_BUG_ON_ZERO(expr).  Every time I
encounter a similarly-named macro in other projects, I have to go
re-read the docs, because (to at least me) the name is not
self-describing.

Quoting the patch itself:

+/* This expression fails at compile time if 'expr' is true.  It does
+ * this by constructing a struct which has an impossible
+ * (negative-sized) array.
+ *
+ * If 'expr' is false then we subtract the sizes of the two identical
+ * structures, returning zero.
+ */

My summary of those docs: BUILD_BUG_ON_ZERO(1) is a build bug (forced
compiler error); BUILD_BUG_ON_ZERO(0) is 0.  To use the macro, I then
have to come up with a condition that results in false to avoid a
build bug (when normally, I like to come up with predicates that
result in true when I want to proceed, rather than predicates that
result in false).

To me, the name "BUILD_BUG_ON_ZERO" parses as "I want a build bug
if I
pass in 0", and not "I want to use 0 unchanged, and anything else to
be a build bug".  And once I get over my mis-parse of the name, I then
have to remember that ARRAY_SIZE must use '+ BUILD_BUG_ON_ZERO
(!TYPE_IS_ARRAY(a))' to mean "add 0 if 'a' is an array type,
because
'!ARRAY_TYPE(a)' is 0", which involved a double negative.

I don't mind if we keep the macro, but can we please use a better name
than the kernel folks have stuck themselves with, especially since we
are NOT copying the kernel's implementation of the macro (as our
licensing won't allow it in the first place)?

I'm also worried that your code uses 'struct { int
_array_size_failed[(expr)...]; }', which may end up causing VLA
situations, where (mistakenly) passing in a non-constant expr could
compile (as a VLA) rather than letting the compiler diagnose that expr
wasn't consant.  Hence, I like bitfields better than array sizes when
trying to force a conditional compiler error, as it gets VLA out of
the picture.

If I were designing it from scratch, I might create:

/* If EXPR is true, produce a non-zero struct size. Otherwise, fail the build */
#define BUILD_BUG_STRUCT_SIZE(expr) \
  (sizeof (struct { int: (expr) ? 1 : -1; }))
/* If EXPR is true, produce 0. Otherwise, fail the build */
#define BUILD_BUG_UNLESS_TRUE(expr) \
  (!BUILD_BUG_STRUCT_SIZE(expr))

at which point the definition of ARRAY_SIZE(a) includes
'+ BUILD_BUG_UNLESS_TRUE(TYPE_IS_ARRAY(a))', which is easier (for me) to
read.

After writing the above, I also looked at how gnulib did things in
lib/verify.h.  Their arugment is that no single macro can be used in
all contexts, so they provide:
/* R must be non-zero, or compilation fails. */
verify_expr(R, E) /* for use in scalar contexts, result in E */
verify(R) /* for use in declaration contexts */

but it matches my bias of avoiding the double-negative (I find it easy
to write a predicate that must either be true or will cause
compilation failure).  Here, I would define ARRAY_SIZE by using
'+ verify_expr(TYPE_IS_ARRAY(a), 0)'

[Caution - I have touched gnulib's lib/verify.h in the past.  Although
much of it has been rewritten in the meantime as compilers have
progressed, I may still be tainted enough by my past work on gnulib's
implementation of compile-time checking that I don't trust myself to
do a genuinely clean-room reimplementation compatible with nbdkit's
licensing needs]

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