| Message ID | 20240715095939.72492-3-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qga/commands-posix: Make ga_wait_child() return boolean | expand |
On 15/7/24 11:59, Zhao Liu wrote: > As the comment in qapi/error, dereferencing @errp requires > ERRP_GUARD(): > > * = Why, when and how to use ERRP_GUARD() = > * > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > * - It must not be dereferenced, because it may be null. > ... > * ERRP_GUARD() lifts these restrictions. > * > * To use ERRP_GUARD(), add it right at the beginning of the function. > * @errp can then be used without worrying about the argument being > * NULL or &error_fatal. > * > * Using it when it's not needed is safe, but please avoid cluttering > * the source with useless code. > > Though currently ga_run_command() only gets &local_err instead of NULL > @errp, it's still better to follow the requirement to add the > ERRP_GUARD(). > > But as error.h suggested, the best practice for callee is to return > something to indicate success / failure. > > So make ga_wait_child() return boolean and check the returned boolean in > ga_run_command() instead of dereferencing @errp, which eliminates the > need of ERRP_GUARD(). I'd avoid mentioning ERRP_GUARD and just describe: Make ga_wait_child() return boolean and check the returned boolean in ga_run_command() instead of dereferencing @errp. For the code change: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Cc: Michael Roth <michael.roth@amd.com> > Cc: Konstantin Kostiuk <kkostiuk@redhat.com> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > qga/commands-posix.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 7f05996495a2..64bb0be94479 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -59,7 +59,7 @@ > #endif > #endif > > -static void ga_wait_child(pid_t pid, int *status, Error **errp) > +static bool ga_wait_child(pid_t pid, int *status, Error **errp) > { > pid_t rpid; > > @@ -70,10 +70,11 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp) > if (rpid == -1) { > error_setg_errno(errp, errno, "failed to wait for child (pid: %d)", > pid); > - return; > + return false; > } > > g_assert(rpid == pid); > + return true; > } > > static ssize_t ga_pipe_read_str(int fd[2], char **str) > @@ -178,8 +179,7 @@ static int ga_run_command(const char *argv[], const char *in_str, > goto out; > } > > - ga_wait_child(pid, &status, errp); > - if (*errp) { > + if (!ga_wait_child(pid, &status, errp)) { > goto out; > } >
On Mon, Jul 15, 2024 at 11:59:29AM +0200, Philippe Mathieu-Daudé wrote: > Date: Mon, 15 Jul 2024 11:59:29 +0200 > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Subject: Re: [PATCH] qga/commands-posix: Make ga_wait_child() return boolean > > On 15/7/24 11:59, Zhao Liu wrote: > > As the comment in qapi/error, dereferencing @errp requires > > ERRP_GUARD(): > > > > * = Why, when and how to use ERRP_GUARD() = > > * > > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > > * - It must not be dereferenced, because it may be null. > > ... > > * ERRP_GUARD() lifts these restrictions. > > * > > * To use ERRP_GUARD(), add it right at the beginning of the function. > > * @errp can then be used without worrying about the argument being > > * NULL or &error_fatal. > > * > > * Using it when it's not needed is safe, but please avoid cluttering > > * the source with useless code. > > > > Though currently ga_run_command() only gets &local_err instead of NULL > > @errp, it's still better to follow the requirement to add the > > ERRP_GUARD(). > > > > But as error.h suggested, the best practice for callee is to return > > something to indicate success / failure. > > > > So make ga_wait_child() return boolean and check the returned boolean in > > ga_run_command() instead of dereferencing @errp, which eliminates the > > need of ERRP_GUARD(). > > I'd avoid mentioning ERRP_GUARD and just describe: > > Make ga_wait_child() return boolean and check the returned boolean > in ga_run_command() instead of dereferencing @errp. > > For the code change: > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Many thanks for your words and review! Will use your words in the next version.
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 7f05996495a2..64bb0be94479 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -59,7 +59,7 @@ #endif #endif -static void ga_wait_child(pid_t pid, int *status, Error **errp) +static bool ga_wait_child(pid_t pid, int *status, Error **errp) { pid_t rpid; @@ -70,10 +70,11 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp) if (rpid == -1) { error_setg_errno(errp, errno, "failed to wait for child (pid: %d)", pid); - return; + return false; } g_assert(rpid == pid); + return true; } static ssize_t ga_pipe_read_str(int fd[2], char **str) @@ -178,8 +179,7 @@ static int ga_run_command(const char *argv[], const char *in_str, goto out; } - ga_wait_child(pid, &status, errp); - if (*errp) { + if (!ga_wait_child(pid, &status, errp)) { goto out; }
As the comment in qapi/error, dereferencing @errp requires ERRP_GUARD(): * = Why, when and how to use ERRP_GUARD() = * * Without ERRP_GUARD(), use of the @errp parameter is restricted: * - It must not be dereferenced, because it may be null. ... * ERRP_GUARD() lifts these restrictions. * * To use ERRP_GUARD(), add it right at the beginning of the function. * @errp can then be used without worrying about the argument being * NULL or &error_fatal. * * Using it when it's not needed is safe, but please avoid cluttering * the source with useless code. Though currently ga_run_command() only gets &local_err instead of NULL @errp, it's still better to follow the requirement to add the ERRP_GUARD(). But as error.h suggested, the best practice for callee is to return something to indicate success / failure. So make ga_wait_child() return boolean and check the returned boolean in ga_run_command() instead of dereferencing @errp, which eliminates the need of ERRP_GUARD(). Cc: Michael Roth <michael.roth@amd.com> Cc: Konstantin Kostiuk <kkostiuk@redhat.com> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- qga/commands-posix.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)