[libvirt] Miscellaneous fixes to build with -Werror
HACKING suggests compiling with --enable-compile-warnings=error before submitting any patches; however, current master fails for me on this account (CentOS 5.3; gcc 4.1.2). Please see attached. I suspect most of these should be uncontroversial -- but wonder if perhaps virStrcpy uses would be better converted to virStrcpyStatic rather than adding virStrcpy to the symbol list as done here, and am curious about whether the need for explicit initialization to silence a warning regarding qemudSetLogging's log_level indicates a bug in the macro later used to assign that value.
Charles Duffy wrote:
HACKING suggests compiling with --enable-compile-warnings=error before submitting any patches; however, current master fails for me on this account (CentOS 5.3; gcc 4.1.2).
Please see attached. I suspect most of these should be uncontroversial -- but wonder if perhaps virStrcpy uses would be better converted to virStrcpyStatic rather than adding virStrcpy to the symbol list as done
That's not possible in general. The problem with virStrcpyStatic is that it *has* to be a macro, and not only that, the users *have* to know that sizeof(src) returns something meaningful. Some callers do not, and cannot, provide for that, so those callsites have to use virStrcpy. I think adding it to the symbol table will have to suffice, although I'm curious about warnings themselves (since I compiled on RHEL-5.4 yesterday, and don't remember seeing any warnings at all). Can you post the warnings that you are seeing? -- Chris Lalancette
Chris Lalancette wrote:
Charles Duffy wrote:
HACKING suggests compiling with --enable-compile-warnings=error before submitting any patches; however, current master fails for me on this account (CentOS 5.3; gcc 4.1.2).
Please see attached. I suspect most of these should be uncontroversial -- but wonder if perhaps virStrcpy uses would be better converted to virStrcpyStatic rather than adding virStrcpy to the symbol list as done
That's not possible in general. The problem with virStrcpyStatic is that it *has* to be a macro, and not only that, the users *have* to know that sizeof(src) returns something meaningful. Some callers do not, and cannot, provide for that, so those callsites have to use virStrcpy. I think adding it to the symbol table will have to suffice, although I'm curious about warnings themselves (since I compiled on RHEL-5.4 yesterday, and don't remember seeing any warnings at all). Can you post the warnings that you are seeing?
Actually, I take this (partially) back. I had forgotten to add on the --enable-compile-warnings=error, so I missed the warnings in the sea of output. So all of your fixes except for the virStrcpy() one I saw, and in point of fact those are all real bugs. (The log_level fix in qemudSetLogging() does also seem right; it's possible that we don't set the var_name in GET_CONF_INT if we don't find it in the config file, so initializing it to 0 seems to be the right thing to do). I didn't see the error about the virStrcpy one, so I would still be curious as to what you are seeing there. -- Chris Lalancette
Chris Lalancette wrote:
I didn't see the error about the virStrcpy one, so I would still be curious as to what you are seeing there.
As the context of the patch implies, this was a link-time issue -- but I find myself unable to reproduce it. I'll continue this thread should that change in the future.
Interesting that gcc-4.41 isn't giving me any warnings On Wed, 2009-09-23 at 12:32 -0500, Charles Duffy wrote:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af215ca..25d983e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6132,7 +6132,7 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - virDomainHostdevDefPtr detach; + virDomainHostdevDefPtr detach = NULL; char *cmd, *reply; int i, ret; pciDevice *pci;
Looks like a very real bug, so I just went ahead and pused this one Thanks, Mark.
Charles Duffy wrote:
HACKING suggests compiling with --enable-compile-warnings=error before submitting any patches; however, current master fails for me on this account (CentOS 5.3; gcc 4.1.2).
Please see attached. I suspect most of these should be uncontroversial -- but wonder if perhaps virStrcpy uses would be better converted to virStrcpyStatic rather than adding virStrcpy to the symbol list as done here, and am curious about whether the need for explicit initialization to silence a warning regarding qemudSetLogging's log_level indicates a bug in the macro later used to assign that value.
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 2bae782..ec2eab1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2492,7 +2492,7 @@ remoteReadSaslAllowedUsernameList (virConfPtr conf ATTRIBUTE_UNUSED, */ static int qemudSetLogging(virConfPtr conf, const char *filename) { - int log_level; + int log_level = 0; char *log_filters = NULL; char *log_outputs = NULL; int ret = -1;
Looking at this more, I'm not sure. The comment above GET_CONF_INT(log_level) looks to be bogus; GET_CONF_INT does *not* return 0 if the value is not in the config file, it doesn't change anything at all. Still, I don't quite know the reasoning behind the original change (back in early August), so I'm uncomfortable changing it.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af215ca..25d983e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6132,7 +6132,7 @@ static int qemudDomainDetachHostPciDevice(virConnectPtr conn, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - virDomainHostdevDefPtr detach; + virDomainHostdevDefPtr detach = NULL; char *cmd, *reply; int i, ret; pciDevice *pci;
Mark McLoughlin pushed this one.
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 20a3fa8..9c4102e 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -432,7 +432,7 @@ static virSecretEntryPtr secretLoad(virConnectPtr conn, virSecretDriverStatePtr driver, const char *xml_basename) { - virSecretDefPtr def; + virSecretDefPtr def = NULL; virSecretEntryPtr secret = NULL, ret = NULL; char *xml_filename;
I just pushed this one because it's an obvious bugfix. -- Chris Lalancette
On Thu, Sep 24, 2009 at 01:02:24PM +0200, Chris Lalancette wrote:
Charles Duffy wrote:
HACKING suggests compiling with --enable-compile-warnings=error before submitting any patches; however, current master fails for me on this account (CentOS 5.3; gcc 4.1.2).
Please see attached. I suspect most of these should be uncontroversial -- but wonder if perhaps virStrcpy uses would be better converted to virStrcpyStatic rather than adding virStrcpy to the symbol list as done here, and am curious about whether the need for explicit initialization to silence a warning regarding qemudSetLogging's log_level indicates a bug in the macro later used to assign that value.
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 2bae782..ec2eab1 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2492,7 +2492,7 @@ remoteReadSaslAllowedUsernameList (virConfPtr conf ATTRIBUTE_UNUSED, */ static int qemudSetLogging(virConfPtr conf, const char *filename) { - int log_level; + int log_level = 0; char *log_filters = NULL; char *log_outputs = NULL; int ret = -1;
Looking at this more, I'm not sure. The comment above GET_CONF_INT(log_level) looks to be bogus; GET_CONF_INT does *not* return 0 if the value is not in the config file, it doesn't change anything at all. Still, I don't quite know the reasoning behind the original change (back in early August), so I'm uncomfortable changing it.
Its pretty clear though that the 'log_level' var will never be initialized if the 'log_level' setting isn't in the config file. ie, GET_CONF_INTconf, filename, log_level); expands to virConfValuePtr p = virConfGetValue (conf, "log_level"); if (p) { if (checkType (p, filename, "log_level", VIR_CONF_LONG) < 0) goto free_and_fail; (log_level) = p->l; } So it is never set if 'p' is NULL 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 :|
participants (4)
-
Charles Duffy -
Chris Lalancette -
Daniel P. Berrange -
Mark McLoughlin