[libvirt] [PATCH] uml_conf.c: don't return an uninitialized pointer
Another "real" bug:
From 6697607bf0b023ffb692576b31d652d10719b08a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 3 Sep 2009 12:05:52 +0200 Subject: [PATCH] uml_conf.c: don't return an uninitialized pointer
* src/uml_conf.c (umlBuildCommandLineChr): Initialize "ret" also in the final switch cases. --- src/uml_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/uml_conf.c b/src/uml_conf.c index 838fedd..2e9c25c 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -331,6 +331,7 @@ umlBuildCommandLineChr(virConnectPtr conn, default: umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unsupported chr device type %d"), def->type); + ret = NULL; break; } -- 1.6.4.2.395.ge3d52
On Thu, Sep 03, 2009 at 12:07:14PM +0200, Jim Meyering wrote:
Another "real" bug:
From 6697607bf0b023ffb692576b31d652d10719b08a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 3 Sep 2009 12:05:52 +0200 Subject: [PATCH] uml_conf.c: don't return an uninitialized pointer
* src/uml_conf.c (umlBuildCommandLineChr): Initialize "ret" also in the final switch cases. --- src/uml_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/uml_conf.c b/src/uml_conf.c index 838fedd..2e9c25c 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -331,6 +331,7 @@ umlBuildCommandLineChr(virConnectPtr conn, default: umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unsupported chr device type %d"), def->type); + ret = NULL; break; }
I think this would be better changing the initial declartion to be initializing to NULL too. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Thu, Sep 03, 2009 at 12:07:14PM +0200, Jim Meyering wrote:
Another "real" bug:
From 6697607bf0b023ffb692576b31d652d10719b08a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 3 Sep 2009 12:05:52 +0200 Subject: [PATCH] uml_conf.c: don't return an uninitialized pointer
* src/uml_conf.c (umlBuildCommandLineChr): Initialize "ret" also in the final switch cases. --- src/uml_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/uml_conf.c b/src/uml_conf.c index 838fedd..2e9c25c 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -331,6 +331,7 @@ umlBuildCommandLineChr(virConnectPtr conn, default: umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unsupported chr device type %d"), def->type); + ret = NULL; break; }
I think this would be better changing the initial declartion to be initializing to NULL too.
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case. With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
On Thu, Sep 03, 2009 at 12:43:10PM +0200, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Thu, Sep 03, 2009 at 12:07:14PM +0200, Jim Meyering wrote:
Another "real" bug:
From 6697607bf0b023ffb692576b31d652d10719b08a Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 3 Sep 2009 12:05:52 +0200 Subject: [PATCH] uml_conf.c: don't return an uninitialized pointer
* src/uml_conf.c (umlBuildCommandLineChr): Initialize "ret" also in the final switch cases. --- src/uml_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/uml_conf.c b/src/uml_conf.c index 838fedd..2e9c25c 100644 --- a/src/uml_conf.c +++ b/src/uml_conf.c @@ -331,6 +331,7 @@ umlBuildCommandLineChr(virConnectPtr conn, default: umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unsupported chr device type %d"), def->type); + ret = NULL; break; }
I think this would be better changing the initial declartion to be initializing to NULL too.
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote: ...
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere
Ok. adjusted
Jim Meyering wrote:
Daniel P. Berrange wrote: ...
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere
Ok. adjusted
Actually, I will now try to convince you that we should do it the other way. Not only is it better to have the compiler tell us about this problem, but if ever someone were to add the definition that seems to be missing in the default case, clang would report the preceding initialization (the one you prefer) as a "dead store" error. In fact, now that I'm looking at what clang calls dead initializations, I see just such a case in mdns.c: static void libvirtd_mdns_client_callback(AvahiClient *c, AvahiClientState state, void *userdata) { ... struct libvirtd_mdns_group *group = mdns->group; ... switch (state) { ... } And in every "case" of that switch, there is an assignment to "group". That renders the preceding assignment useless, and hence clang calls it a dead initialization. Just to show you that they're not all so ambiguous, node_device_conf.c:116 has a "dead initialization" that is obviously worth fixing: virNodeDevCapsDefPtr caps = def->caps; ... for (caps = def->caps; caps; caps = caps->next) { Posting that separately.
Jim Meyering wrote:
Jim Meyering wrote:
Daniel P. Berrange wrote: ...
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere
Ok. adjusted
Actually, I will now try to convince you that we should do it the other way. Not only is it better to have the compiler tell us about this problem, but if ever someone were to add the definition that seems to be missing in the default case, clang would report the preceding initialization (the one you prefer) as a "dead store" error.
In fact, now that I'm looking at what clang calls dead initializations, I see just such a case in mdns.c:
static void libvirtd_mdns_client_callback(AvahiClient *c, AvahiClientState state, void *userdata) { ... struct libvirtd_mdns_group *group = mdns->group; ...
switch (state) { ... }
And in every "case" of that switch, there is an assignment to "group". That renders the preceding assignment useless, and hence clang calls it a dead initialization.
Oops. That was inaccurate. Each case does not set group. However, each following *use* of group is preceded by another assignment to that variable. The point stands though.
On Thu, Sep 03, 2009 at 01:39:25PM +0200, Jim Meyering wrote:
Jim Meyering wrote:
Daniel P. Berrange wrote: ...
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere
Ok. adjusted
Actually, I will now try to convince you that we should do it the other way. Not only is it better to have the compiler tell us about this problem, but if ever someone were to add the definition that seems to be missing in the default case, clang would report the preceding initialization (the one you prefer) as a "dead store" error.
This isn't a dead store though - we are setting the default return value for cases where there's no explicit return value set. It would only become a dead store if we also added the redundant 'ret = NULL' initialization that your initial patch did.
In fact, now that I'm looking at what clang calls dead initializations, I see just such a case in mdns.c:
These are different scenarios dealing with local use variables only, rather tha default return values as the above case. Removing these redundant inits makes sense for mdns/nodedev. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Sep 03, 2009 at 01:39:25PM +0200, Jim Meyering wrote:
Jim Meyering wrote:
Daniel P. Berrange wrote: ...
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere
Ok. adjusted
Actually, I will now try to convince you that we should do it the other way. Not only is it better to have the compiler tell us about this problem, but if ever someone were to add the definition that seems to be missing in the default case, clang would report the preceding initialization (the one you prefer) as a "dead store" error.
[...]
And in every "case" of that switch, there is an assignment to "group". That renders the preceding assignment useless, and hence clang calls it a dead initialization.
Just to show you that they're not all so ambiguous, node_device_conf.c:116 has a "dead initialization" that is obviously worth fixing:
virNodeDevCapsDefPtr caps = def->caps; ... for (caps = def->caps; caps; caps = caps->next) {
Posting that separately.
I must admit I don't see a NULL initialization of a pointer for safety when entering a routine as a bad thing, actually this could be considered a standard feature in any highlevel language. Assignment of a computed value is rather different. So while I agree with your example I still think it's fine to initialize the pointer to NULL in the patch. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Thu, Sep 03, 2009 at 01:39:25PM +0200, Jim Meyering wrote:
Jim Meyering wrote:
Daniel P. Berrange wrote: ...
Actually I did that first, but then un-did it in favor of the change above. Why? because that initialization could mask a failure to initialize in a new case.
With per-case initialization, we'd detect the bug at compile/static-analysis time. With the up-front unconditional initialization, we cannot, and would have to rely on testing to find it.
It is a tradeoff, but I still prefer the initialization at time of declaration as a safety net, and we do use this pattern pretty much everywhere
Ok. adjusted
Actually, I will now try to convince you that we should do it the other way. Not only is it better to have the compiler tell us about this problem, but if ever someone were to add the definition that seems to be missing in the default case, clang would report the preceding initialization (the one you prefer) as a "dead store" error.
[...]
And in every "case" of that switch, there is an assignment to "group". That renders the preceding assignment useless, and hence clang calls it a dead initialization.
Just to show you that they're not all so ambiguous, node_device_conf.c:116 has a "dead initialization" that is obviously worth fixing:
virNodeDevCapsDefPtr caps = def->caps; ... for (caps = def->caps; caps; caps = caps->next) {
Posting that separately.
I must admit I don't see a NULL initialization of a pointer for safety when entering a routine as a bad thing, actually this could be considered a standard feature in any highlevel language.
Then perhaps I didn't explain well. Here's a small example: int foo (void) { int ret = 0; switch (f()) { case 0: do_something (); ret = -1; break; case 1: do_something_else (); ret = -1; break; default: ret = -2; break; } return ret; } In that function, the initialization of "ret" is officially unnecessary. Or, make it more like the code in question, initializing to the value used in the "default" case: int foo (void) { int ret = -2; switch (f()) { case 0: do_something (); ret = -1; break; case 1: do_something_else (); ret = -1; break; default: break; } return ret; } However, Dan argued that he wants to be sure "ret" is defined, in case someone else (one of the case stmts) forgets. However, that's precisely the case in which you do *not* want there to be an overarching definition, because with that "extra", initial assignment, the compiler cannot warn you if you add a case like this: case 2: // ...do things, but forget to set "ret" break; However, if you merely declare "ret", with no initial value, adding that erroneous "case 2" would provoke a warning about "ret" being used (via the return) undefined. I prefer to use the compiler as a safety net, whenever possible. I.e., imho, it is clear that this toy function should be written like this, with no initial assignment to "ret": int foo (void) { int ret; switch (f()) { case 0: do_something (); ret = -1; break; case 1: do_something_else (); ret = -1; break; default: ret = -2; break; } return ret; }
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering