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

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[] = {
+  "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);
-- 
2.37.0.rc2

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