diff mbox series

[16/31] seccomp: Clean up error reporting in parse_sandbox()

Message ID 20181008173125.19678-17-armbru@redhat.com
State New
Headers show
Series Replace some unwise uses of error_report() & friends | expand

Commit Message

Markus Armbruster Oct. 8, 2018, 5:31 p.m. UTC 
Calling error_report() in a function that takes an Error ** argument
is suspicious.  parse_sandbox() does that, and then fails without
setting an error.  Its caller main(), via qemu_opts_foreach(), is fine
with it, but clean it up anyway.

Cc: Eduardo Otubo <otubo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-seccomp.c | 18 +++++++++---------
 vl.c           |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Marc-André Lureau Oct. 9, 2018, 9:53 a.m. UTC | #1 
Hi

On Mon, Oct 8, 2018 at 9:50 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Calling error_report() in a function that takes an Error ** argument
> is suspicious.  parse_sandbox() does that, and then fails without
> setting an error.  Its caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway.
>
> Cc: Eduardo Otubo <otubo@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-seccomp.c | 18 +++++++++---------
>  vl.c           |  4 ++--
>  2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 1baa5c69ed..6d27699409 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -12,11 +12,12 @@
>   * Contributions after 2012-01-13 are licensed under the terms of the
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
> +
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "qemu/config-file.h"
>  #include "qemu/option.h"
>  #include "qemu/module.h"
> -#include "qemu/error-report.h"
>  #include <sys/prctl.h>
>  #include <seccomp.h>
>  #include "sysemu/seccomp.h"
> @@ -190,7 +191,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>                   * to provide a little bit of consistency for
>                   * the command line */
>              } else {
> -                error_report("invalid argument for obsolete");
> +                error_setg(errp, "invalid argument for obsolete");
>                  return -1;
>              }
>          }
> @@ -205,14 +206,13 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>                  /* calling prctl directly because we're
>                   * not sure if host has CAP_SYS_ADMIN set*/
>                  if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
> -                    error_report("failed to set no_new_privs "
> -                                 "aborting");
> +                    error_setg(errp, "failed to set no_new_privs " "aborting");

Extra " "

>                      return -1;
>                  }
>              } else if (g_str_equal(value, "allow")) {
>                  /* default value */
>              } else {
> -                error_report("invalid argument for elevateprivileges");
> +                error_setg(errp, "invalid argument for elevateprivileges");
>                  return -1;
>              }
>          }
> @@ -224,7 +224,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>              } else if (g_str_equal(value, "allow")) {
>                  /* default value */
>              } else {
> -                error_report("invalid argument for spawn");
> +                error_setg(errp, "invalid argument for spawn");
>                  return -1;
>              }
>          }
> @@ -236,14 +236,14 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>              } else if (g_str_equal(value, "allow")) {
>                  /* default value */
>              } else {
> -                error_report("invalid argument for resourcecontrol");
> +                error_setg(errp, "invalid argument for resourcecontrol");
>                  return -1;
>              }
>          }
>
>          if (seccomp_start(seccomp_opts) < 0) {
> -            error_report("failed to install seccomp syscall filter "
> -                         "in the kernel");
> +            error_setg(errp, "failed to install seccomp syscall filter "
> +                       "in the kernel");
>              return -1;
>          }
>      }
> diff --git a/vl.c b/vl.c
> index 9d2b38a31f..485c3fc008 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3925,8 +3925,8 @@ int main(int argc, char **argv, char **envp)
>
>  #ifdef CONFIG_SECCOMP
>      olist = qemu_find_opts_err("sandbox", NULL);
> -    if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
> -        exit(1);
> +    if (olist) {
> +        qemu_opts_foreach(olist, parse_sandbox, NULL, &error_fatal);
>      }
>  #endif
>

Other than that,


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> --
> 2.17.1
>
>
Eduardo Otubo Oct. 10, 2018, 12:39 p.m. UTC | #2 
On 08/10/2018 - 19:31:10, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious.  parse_sandbox() does that, and then fails without
> setting an error.  Its caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway.
> 
> Cc: Eduardo Otubo <otubo@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-seccomp.c | 18 +++++++++---------
>  vl.c           |  4 ++--
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 1baa5c69ed..6d27699409 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -12,11 +12,12 @@
>   * Contributions after 2012-01-13 are licensed under the terms of the
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
> +
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "qemu/config-file.h"
>  #include "qemu/option.h"
>  #include "qemu/module.h"
> -#include "qemu/error-report.h"
>  #include <sys/prctl.h>
>  #include <seccomp.h>
>  #include "sysemu/seccomp.h"
> @@ -190,7 +191,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>                   * to provide a little bit of consistency for
>                   * the command line */
>              } else {
> -                error_report("invalid argument for obsolete");
> +                error_setg(errp, "invalid argument for obsolete");
>                  return -1;
>              }
>          }
> @@ -205,14 +206,13 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>                  /* calling prctl directly because we're
>                   * not sure if host has CAP_SYS_ADMIN set*/
>                  if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
> -                    error_report("failed to set no_new_privs "
> -                                 "aborting");
> +                    error_setg(errp, "failed to set no_new_privs " "aborting");
>                      return -1;

Except for this " " all else is good.

Acked-by: Eduardo Otubo <otubo@redhat.com>

>                  }
>              } else if (g_str_equal(value, "allow")) {
>                  /* default value */
>              } else {
> -                error_report("invalid argument for elevateprivileges");
> +                error_setg(errp, "invalid argument for elevateprivileges");
>                  return -1;
>              }
>          }
> @@ -224,7 +224,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>              } else if (g_str_equal(value, "allow")) {
>                  /* default value */
>              } else {
> -                error_report("invalid argument for spawn");
> +                error_setg(errp, "invalid argument for spawn");
>                  return -1;
>              }
>          }
> @@ -236,14 +236,14 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>              } else if (g_str_equal(value, "allow")) {
>                  /* default value */
>              } else {
> -                error_report("invalid argument for resourcecontrol");
> +                error_setg(errp, "invalid argument for resourcecontrol");
>                  return -1;
>              }
>          }
>  
>          if (seccomp_start(seccomp_opts) < 0) {
> -            error_report("failed to install seccomp syscall filter "
> -                         "in the kernel");
> +            error_setg(errp, "failed to install seccomp syscall filter "
> +                       "in the kernel");
>              return -1;
>          }
>      }
> diff --git a/vl.c b/vl.c
> index 9d2b38a31f..485c3fc008 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3925,8 +3925,8 @@ int main(int argc, char **argv, char **envp)
>  
>  #ifdef CONFIG_SECCOMP
>      olist = qemu_find_opts_err("sandbox", NULL);
> -    if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
> -        exit(1);
> +    if (olist) {
> +        qemu_opts_foreach(olist, parse_sandbox, NULL, &error_fatal);
>      }
>  #endif
>  
> -- 
> 2.17.1
>
Markus Armbruster Oct. 11, 2018, 5:40 p.m. UTC | #3 
Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Oct 8, 2018 at 9:50 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Calling error_report() in a function that takes an Error ** argument
>> is suspicious.  parse_sandbox() does that, and then fails without
>> setting an error.  Its caller main(), via qemu_opts_foreach(), is fine
>> with it, but clean it up anyway.
>>
>> Cc: Eduardo Otubo <otubo@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qemu-seccomp.c | 18 +++++++++---------
>>  vl.c           |  4 ++--
>>  2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
>> index 1baa5c69ed..6d27699409 100644
>> --- a/qemu-seccomp.c
>> +++ b/qemu-seccomp.c
>> @@ -12,11 +12,12 @@
>>   * Contributions after 2012-01-13 are licensed under the terms of the
>>   * GNU GPL, version 2 or (at your option) any later version.
>>   */
>> +
>>  #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>  #include "qemu/config-file.h"
>>  #include "qemu/option.h"
>>  #include "qemu/module.h"
>> -#include "qemu/error-report.h"
>>  #include <sys/prctl.h>
>>  #include <seccomp.h>
>>  #include "sysemu/seccomp.h"
>> @@ -190,7 +191,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>>                   * to provide a little bit of consistency for
>>                   * the command line */
>>              } else {
>> -                error_report("invalid argument for obsolete");
>> +                error_setg(errp, "invalid argument for obsolete");
>>                  return -1;
>>              }
>>          }
>> @@ -205,14 +206,13 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>>                  /* calling prctl directly because we're
>>                   * not sure if host has CAP_SYS_ADMIN set*/
>>                  if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
>> -                    error_report("failed to set no_new_privs "
>> -                                 "aborting");
>> +                    error_setg(errp, "failed to set no_new_privs " "aborting");
>
> Extra " "

Fixing...

>>                      return -1;
>>                  }
>>              } else if (g_str_equal(value, "allow")) {
>>                  /* default value */
>>              } else {
>> -                error_report("invalid argument for elevateprivileges");
>> +                error_setg(errp, "invalid argument for elevateprivileges");
>>                  return -1;
>>              }
>>          }
>> @@ -224,7 +224,7 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>>              } else if (g_str_equal(value, "allow")) {
>>                  /* default value */
>>              } else {
>> -                error_report("invalid argument for spawn");
>> +                error_setg(errp, "invalid argument for spawn");
>>                  return -1;
>>              }
>>          }
>> @@ -236,14 +236,14 @@ int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
>>              } else if (g_str_equal(value, "allow")) {
>>                  /* default value */
>>              } else {
>> -                error_report("invalid argument for resourcecontrol");
>> +                error_setg(errp, "invalid argument for resourcecontrol");
>>                  return -1;
>>              }
>>          }
>>
>>          if (seccomp_start(seccomp_opts) < 0) {
>> -            error_report("failed to install seccomp syscall filter "
>> -                         "in the kernel");
>> +            error_setg(errp, "failed to install seccomp syscall filter "
>> +                       "in the kernel");
>>              return -1;
>>          }
>>      }
>> diff --git a/vl.c b/vl.c
>> index 9d2b38a31f..485c3fc008 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3925,8 +3925,8 @@ int main(int argc, char **argv, char **envp)
>>
>>  #ifdef CONFIG_SECCOMP
>>      olist = qemu_find_opts_err("sandbox", NULL);
>> -    if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
>> -        exit(1);
>> +    if (olist) {
>> +        qemu_opts_foreach(olist, parse_sandbox, NULL, &error_fatal);
>>      }
>>  #endif
>>
>
> Other than that,
>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!
diff mbox series

Patch

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 1baa5c69ed..6d27699409 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -12,11 +12,12 @@ 
  * Contributions after 2012-01-13 are licensed under the terms of the
  * GNU GPL, version 2 or (at your option) any later version.
  */
+
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/module.h"
-#include "qemu/error-report.h"
 #include <sys/prctl.h>
 #include <seccomp.h>
 #include "sysemu/seccomp.h"
@@ -190,7 +191,7 @@  int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
                  * to provide a little bit of consistency for
                  * the command line */
             } else {
-                error_report("invalid argument for obsolete");
+                error_setg(errp, "invalid argument for obsolete");
                 return -1;
             }
         }
@@ -205,14 +206,13 @@  int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
                 /* calling prctl directly because we're
                  * not sure if host has CAP_SYS_ADMIN set*/
                 if (prctl(PR_SET_NO_NEW_PRIVS, 1)) {
-                    error_report("failed to set no_new_privs "
-                                 "aborting");
+                    error_setg(errp, "failed to set no_new_privs " "aborting");
                     return -1;
                 }
             } else if (g_str_equal(value, "allow")) {
                 /* default value */
             } else {
-                error_report("invalid argument for elevateprivileges");
+                error_setg(errp, "invalid argument for elevateprivileges");
                 return -1;
             }
         }
@@ -224,7 +224,7 @@  int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
             } else if (g_str_equal(value, "allow")) {
                 /* default value */
             } else {
-                error_report("invalid argument for spawn");
+                error_setg(errp, "invalid argument for spawn");
                 return -1;
             }
         }
@@ -236,14 +236,14 @@  int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
             } else if (g_str_equal(value, "allow")) {
                 /* default value */
             } else {
-                error_report("invalid argument for resourcecontrol");
+                error_setg(errp, "invalid argument for resourcecontrol");
                 return -1;
             }
         }
 
         if (seccomp_start(seccomp_opts) < 0) {
-            error_report("failed to install seccomp syscall filter "
-                         "in the kernel");
+            error_setg(errp, "failed to install seccomp syscall filter "
+                       "in the kernel");
             return -1;
         }
     }
diff --git a/vl.c b/vl.c
index 9d2b38a31f..485c3fc008 100644
--- a/vl.c
+++ b/vl.c
@@ -3925,8 +3925,8 @@  int main(int argc, char **argv, char **envp)
 
 #ifdef CONFIG_SECCOMP
     olist = qemu_find_opts_err("sandbox", NULL);
-    if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
-        exit(1);
+    if (olist) {
+        qemu_opts_foreach(olist, parse_sandbox, NULL, &error_fatal);
     }
 #endif