[libvirt] [PATCH 0/7] Couple of -Os fixes
So while trying to bring down the size of my nss module, one of the things I've tried was compiling with -Os instead of -O3. Interesting errors pop up. Here are the fixes. If you think I should squash some patches together, I will gladly do that. Michal Privoznik (7): virCommandAddEnv: Drop inline virDomainNumatuneNodeSpecified: Drop inline virDomainChrRemove: Initialize @ret vbox: Initialize @rc qemuDomainBlockCommit: Initialize @baseSource virNetworkDefForwardIf: drop inline virStorageBackendLogicalMatchPoolSource: Initialize @thisSource src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 8 ++++++++ src/conf/network_conf.h | 8 +------- src/conf/numa_conf.c | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/util/vircommand.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- 10 files changed, 17 insertions(+), 14 deletions(-) -- 2.4.10
This function is big enough for the compiler to be not inlined. This is the error message I'm seeing: util/vircommand.c: In function 'virCommandAddEnvFormat': util/vircommand.c:1257:1: error: inlining failed in call to 'virCommandAddEnv': call is unlikely and code size would grow [-Werror=inline] virCommandAddEnv(virCommandPtr cmd, char *env) ^ util/vircommand.c:1308:5: error: called from here [-Werror=inline] virCommandAddEnv(cmd, env); ^ Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index fe7bf34..027cb64 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1253,7 +1253,7 @@ virCommandRawStatus(virCommandPtr cmd) * string like "name=value". If the named environment variable is * already set, then it is replaced in the list. */ -static inline void +static void virCommandAddEnv(virCommandPtr cmd, char *env) { size_t namelen; -- 2.4.10
The function is exported and called from other places. It shouldn't be inlined then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/numa_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index b5963ac..e0d5688 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -72,7 +72,7 @@ struct _virDomainNuma { }; -inline bool +bool virDomainNumatuneNodeSpecified(virDomainNumaPtr numatune, int cellid) { -- 2.4.10
I've seen the following error message during compilation: conf/domain_conf.c: In function 'virDomainChrRemove': conf/domain_conf.c:13666:24: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized] virDomainChrDefPtr ret, **arrPtr = NULL; ^ Compiler fails to see that @ret is used only if set in the loop, but whatever, there's no harm in initializing the variable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8bfe895..39cedbd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13663,7 +13663,7 @@ virDomainChrDefPtr virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr) { - virDomainChrDefPtr ret, **arrPtr = NULL; + virDomainChrDefPtr ret = NULL, **arrPtr = NULL; size_t i, *cntPtr = NULL; virDomainChrGetDomainPtrsInternal(vmdef, chr->deviceType, &arrPtr, &cntPtr); -- 2.4.10
In vboxAttachDrivesNew and _vboxAttachDrivesOld compiler thinks that @rc may be used uninitialized. Well, not directly, but maybe after some optimization. Yet again, no harm in initializing a variable. In file included from ./util/virthread.h:26:0, from ./datatypes.h:28, from vbox/vbox_tmpl.c:43, from vbox/vbox_V3_1.c:37: vbox/vbox_tmpl.c: In function '_vboxAttachDrivesOld': ./util/virerror.h:181:5: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized] virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ ^ In file included from vbox/vbox_V3_1.c:37:0: vbox/vbox_tmpl.c:1041:14: note: 'rc' was declared here nsresult rc; ^ cc1: all warnings being treated as errors Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 8c00a4f..0152b35 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -987,7 +987,7 @@ vboxAttachDrivesNew(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine { /* AttachDrives for 3.0 and later */ size_t i; - nsresult rc; + nsresult rc = 0; PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; PRUnichar *storageCtlName = NULL; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index cc86bf7..0fbd5b3 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1038,7 +1038,7 @@ static void _vboxAttachDrivesOld(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) { size_t i; - nsresult rc; + nsresult rc = 0; PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; -- 2.4.10
Yet again, one uninitialized variable. qemu/qemu_driver.c: In function 'qemuDomainBlockCommit': qemu/qemu_driver.c:17194:9: error: 'baseSource' may be used uninitialized in this function [-Werror=maybe-uninitialized] qemuDomainPrepareDiskChainElement(driver, vm, baseSource, ^ Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4bd4071..04f9700 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16982,7 +16982,7 @@ qemuDomainBlockCommit(virDomainPtr dom, virDomainDiskDefPtr disk = NULL; virStorageSourcePtr topSource; unsigned int topIndex = 0; - virStorageSourcePtr baseSource; + virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; virStorageSourcePtr top_parent = NULL; bool clean_access = false; -- 2.4.10
In file included from network/bridge_driver_platform.h:30:0, from network/bridge_driver_platform.c:26: network/bridge_driver_linux.c: In function 'networkRemoveRoutingFirewallRules': ./conf/network_conf.h:350:1: error: inlining failed in call to 'virNetworkDefForwardIf.constprop': call is unlikely and code size would grow [-Werror=inline] virNetworkDefForwardIf(const virNetworkDef *def, size_t n) ^ Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 8 ++++++++ src/conf/network_conf.h | 8 +------- src/libvirt_private.syms | 1 + 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6300178..4fb2e2a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2925,6 +2925,14 @@ virNetworkObjFormat(virNetworkObjPtr net, return NULL; } +const char * +virNetworkDefForwardIf(const virNetworkDef *def, size_t n) +{ + return ((def->forward.ifs && (def->forward.nifs > n) && + def->forward.ifs[n].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV) + ? def->forward.ifs[n].device.dev : NULL); +} + virPortGroupDefPtr virPortGroupFindByName(virNetworkDefPtr net, const char *portgroup) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1cd5100..b72257b 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -346,13 +346,7 @@ int virNetworkDefFormatBuf(virBufferPtr buf, const virNetworkDef *def, unsigned int flags); -static inline const char * -virNetworkDefForwardIf(const virNetworkDef *def, size_t n) -{ - return ((def->forward.ifs && (def->forward.nifs > n) && - def->forward.ifs[n].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV) - ? def->forward.ifs[n].device.dev : NULL); -} +const char * virNetworkDefForwardIf(const virNetworkDef *def, size_t n); virPortGroupDefPtr virPortGroupFindByName(virNetworkDefPtr net, const char *portgroup); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5251dc0..3a1b9e1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -589,6 +589,7 @@ virNetworkConfigFile; virNetworkDefCopy; virNetworkDefFormat; virNetworkDefFormatBuf; +virNetworkDefForwardIf; virNetworkDefFree; virNetworkDefGetIpByIndex; virNetworkDefGetRouteByIndex; -- 2.4.10
On Thu, Mar 03, 2016 at 09:44:41AM +0100, Michal Privoznik wrote:
In file included from network/bridge_driver_platform.h:30:0, from network/bridge_driver_platform.c:26: network/bridge_driver_linux.c: In function 'networkRemoveRoutingFirewallRules': ./conf/network_conf.h:350:1: error: inlining failed in call to 'virNetworkDefForwardIf.constprop': call is unlikely and code size would grow [-Werror=inline] virNetworkDefForwardIf(const virNetworkDef *def, size_t n) ^ Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Double sign-off; not only drop inline, but make it as non-static function. To those that might wonder why isn't only the static keyword removed, that doesn't work, I tried it, but I cannot explain it. This is the same patch I came up with, except the double sign-off ;)
storage/storage_backend_logical.c: In function 'virStorageBackendLogicalMatchPoolSource.isra.2': storage/storage_backend_logical.c:618:33: error: 'thisSource' may be used uninitialized in this function [-Werror=maybe-uninitialized] thisSource->devices[j].path)) ^ Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_logical.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 167fe58..ecbf430 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -575,7 +575,7 @@ static bool virStorageBackendLogicalMatchPoolSource(virStoragePoolObjPtr pool) { virStoragePoolSourceList sourceList; - virStoragePoolSource *thisSource; + virStoragePoolSource *thisSource = NULL; size_t i, j; int matchcount = 0; bool ret = false; -- 2.4.10
On Thu, Mar 03, 2016 at 09:44:35AM +0100, Michal Privoznik wrote:
So while trying to bring down the size of my nss module, one of the things I've tried was compiling with -Os instead of -O3. Interesting errors pop up. Here are the fixes. If you think I should squash some patches together, I will gladly do that.
I would squash together initialization patches and then patches that deal with the inlines. ACK series with the removed double sign-off in 6/7.
Michal Privoznik (7): virCommandAddEnv: Drop inline virDomainNumatuneNodeSpecified: Drop inline virDomainChrRemove: Initialize @ret vbox: Initialize @rc qemuDomainBlockCommit: Initialize @baseSource virNetworkDefForwardIf: drop inline virStorageBackendLogicalMatchPoolSource: Initialize @thisSource
src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 8 ++++++++ src/conf/network_conf.h | 8 +------- src/conf/numa_conf.c | 2 +- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/util/vircommand.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- 10 files changed, 17 insertions(+), 14 deletions(-)
-- 2.4.10
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Michal Privoznik