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é <phi...@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é <phi...@linaro.org>
Many thanks for your words and review! Will use your words in the next version.
