* [PATCH] vl: Fix an assert failure in error path @ 2021-06-10 8:47 Zhenzhong Duan 2021-06-09 9:30 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Zhenzhong Duan @ 2021-06-10 8:47 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, Zhenzhong Duan 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; os_daemonize(); rcu_disable_atfork(); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-10 8:47 [PATCH] vl: Fix an assert failure in error path Zhenzhong Duan @ 2021-06-09 9:30 ` Paolo Bonzini 2021-06-09 12:09 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2021-06-09 9:30 UTC (permalink / raw) To: Zhenzhong Duan, qemu-devel 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; > > os_daemonize(); > rcu_disable_atfork(); > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-09 9:30 ` Paolo Bonzini @ 2021-06-09 12:09 ` Markus Armbruster 2021-06-09 12:15 ` Daniel P. Berrangé 2021-06-10 12:55 ` Paolo Bonzini 0 siblings, 2 replies; 8+ messages in thread From: Markus Armbruster @ 2021-06-09 12:09 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Zhenzhong Duan, qemu-devel 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. >> os_daemonize(); >> rcu_disable_atfork(); >> > > Queued, thanks. Suggest to amend the commit message with Fixes: 03d2b412aaf2078425f8472f31c8a9c2340969eb ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-09 12:09 ` Markus Armbruster @ 2021-06-09 12:15 ` Daniel P. Berrangé 2021-06-10 1:47 ` Peng Liang 2021-06-10 12:55 ` Paolo Bonzini 1 sibling, 1 reply; 8+ messages in thread From: Daniel P. Berrangé @ 2021-06-09 12:15 UTC (permalink / raw) To: Markus Armbruster; +Cc: Paolo Bonzini, Zhenzhong Duan, qemu-devel 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 -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-09 12:15 ` Daniel P. Berrangé @ 2021-06-10 1:47 ` Peng Liang 2021-06-10 7:32 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Peng Liang @ 2021-06-10 1:47 UTC (permalink / raw) To: Daniel P. Berrangé, Markus Armbruster Cc: Paolo Bonzini, Zhenzhong Duan, qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-10 1:47 ` Peng Liang @ 2021-06-10 7:32 ` Markus Armbruster 2021-06-10 13:29 ` Peng Liang 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2021-06-10 7:32 UTC (permalink / raw) To: Peng Liang Cc: Paolo Bonzini, Daniel P. Berrangé, Zhenzhong Duan, qemu-devel Peng Liang <liangpeng10@huawei.com> writes: > 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.: Coccinelle is good for finding and fixing instances of bad patterns that have crept in. checkpatch is good for keeping them out. Both has its uses. > @ 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. Crash bug, broken in commit efd7ab22fb "vl: extract qemu_init_subsystems", v6.0.0. Care to submit a fix? > 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. Excluding util/error.c is easy once you know how to: @ depends on !(file in "util/error.c")@ identifier id; @@ ... Excluding variable definitions with static storage duraction shouldn't be too hard, either. If Coccinelle is sufficiently clever, adding keyword auto suffices. Else, try matching keyword static separately, like so: ( static Error *id; | - Error *id; + Error *id = NULL; ) Completely untested. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-10 7:32 ` Markus Armbruster @ 2021-06-10 13:29 ` Peng Liang 0 siblings, 0 replies; 8+ messages in thread From: Peng Liang @ 2021-06-10 13:29 UTC (permalink / raw) To: Markus Armbruster Cc: Paolo Bonzini, Daniel P. Berrangé, Zhenzhong Duan, qemu-devel On 6/10/2021 3:32 PM, Markus Armbruster wrote: > Peng Liang <liangpeng10@huawei.com> writes: > >> 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.: > > Coccinelle is good for finding and fixing instances of bad patterns that > have crept in. checkpatch is good for keeping them out. Both has its > uses. > >> @ 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. > > Crash bug, broken in commit efd7ab22fb "vl: extract > qemu_init_subsystems", v6.0.0. Care to submit a fix? Have sent :) > >> 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. > > Excluding util/error.c is easy once you know how to: > > @ depends on !(file in "util/error.c")@ > identifier id; > @@ > ... > > Excluding variable definitions with static storage duraction shouldn't > be too hard, either. If Coccinelle is sufficiently clever, adding > keyword auto suffices. Else, try matching keyword static separately, > like so: > > ( > static Error *id; > | > - Error *id; > + Error *id = NULL; > ) > > Completely untested. > Thanks for your improvement! Thanks, Peng ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vl: Fix an assert failure in error path 2021-06-09 12:09 ` Markus Armbruster 2021-06-09 12:15 ` Daniel P. Berrangé @ 2021-06-10 12:55 ` Paolo Bonzini 1 sibling, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2021-06-10 12:55 UTC (permalink / raw) To: Markus Armbruster; +Cc: Zhenzhong Duan, qemu-devel On 09/06/21 14:09, Markus Armbruster wrote: > Fixes: 03d2b412aaf2078425f8472f31c8a9c2340969eb Actually 0546c0609c ("vl: split various early command line options to a separate function", 2020-12-10). Done, thanks! Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-10 13:32 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-10 8:47 [PATCH] vl: Fix an assert failure in error path Zhenzhong Duan 2021-06-09 9:30 ` Paolo Bonzini 2021-06-09 12:09 ` Markus Armbruster 2021-06-09 12:15 ` Daniel P. Berrangé 2021-06-10 1:47 ` Peng Liang 2021-06-10 7:32 ` Markus Armbruster 2021-06-10 13:29 ` Peng Liang 2021-06-10 12:55 ` Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.