Re: [PATCH] vl: Fix an assert failure in error path

[Peng Liang <liangpeng10@huawei.com>]

On 6/9/2021 8:15 PM, Daniel P. Berrangé wrote:
> On Wed, Jun 09, 2021 at 02:09:47PM +0200, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 10/06/21 10:47, Zhenzhong Duan wrote:
>>>> Based on the description of error_setg(), the local variable err in
>>>> qemu_maybe_daemonize() should be initialized to NULL.
>>>> Without fix, the uninitialized *errp triggers assert failure which
>>>> doesn't show much valuable information.
>>>> Before the fix:
>>>> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
>>>> After fix:
>>>> qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>   softmmu/vl.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>>>> index 326c1e9080..feb4d201f3 100644
>>>> --- a/softmmu/vl.c
>>>> +++ b/softmmu/vl.c
>>>> @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void)
>>>>     static void qemu_maybe_daemonize(const char *pid_file)
>>>>   {
>>>> -    Error *err;
>>>> +    Error *err = NULL;
>>
>> Common mistake, I'm afraid.
> 
> Initializing isn't likely to be a performance impact, so I'd think
> we should make 'checkpatch.pl' complain about any 'Error *' variable
> that is not initialized to NULL, as a safety net, even if not technically
> required in some cases.
> 
> Regards,
> Daniel
> 

Hi,
Could we add a coccinelle script to check (and fix) these problems?  e.g.:
@ r @
identifier id;
@@
  Error *id
+ = NULL
  ;

Using this script, I found that local variable err in
qemu_init_subsystems is not initialized to NULL too.  The script is not
prefect though, it will initialize all global/static 'Error *' variables
and all local 'Error *' variables in util/error.c to NULL, which is
unnecessary.

Thanks,
Peng