6:57 p.m.
On 02/23/2015 03:12 PM, Laine Stump wrote:
The patch I posted failed to pass make check for two reasons:
1) There are valid use cases when the interface object is
type='ethernet' but has no ifname. Apparently if you provide an "ifup"
script name for -netdev but don't specify a tap device name, qemu will
create a tap device for you, and in that case of course libvirt would be
unable to provide the name to systemd.
2) Even if we avoid the code to look for the ifindex when ifname is
NULL (see (1), make check will *still* fail because there are tests in
the suite that have type='ethernet' and still have an ifname
specified, but that device of course doesn't actually exist on the
test system, so attempts to call virNetDevGetIndex() will
fail. The solution here is to change qemuBuildInterfaceCommandline()
so that it won't even try to add anything to the nicindexes array if
NULL is sent in the args, and modify the calls from test programs to
do exactly that.
I intend to squash this patch into the original, already acked by danpb.
---
src/qemu/qemu_command.c | 13 ++++++-------
src/qemu/qemu_driver.c | 6 +-----
tests/qemuxml2argvtest.c | 5 +----
tests/qemuxmlnstest.c | 5 +----
4 files changed, 9 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 81f6982..1e63905 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7840,10 +7840,12 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
Since you're merging anyway... The comment to the switch needs some
adjustment:
+ /* For types whose implementions use a netdev on the host, add an
+ * entry to nicifindexes for passing on to systemd.
+ */
-> s/implementions/implementations
-> s/nicifindexes/nicindexes
-> s/'*/'/' */'/, e.g. needs one extra space to be properly aligned
/* network and bridge use a tap device, and direct uses a
* macvtap device
*/
- if (virNetDevGetIndex(net->ifname, &nicindex) < 0 ||
- VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex) < 0)
- goto cleanup;
- break;
+ if (nicindexes && nnicindexes && net->ifname) {
^^ Adding the net->ifname check sets off the Coverity FORWARD_NULL check
later on in the following code
7874 if (actualBandwidth) {
7875 if (virNetDevSupportBandwidth(actualType)) {
7876 if (virNetDevBandwidthSet(net->ifname,
actualBandwidth, false) < 0)
It doesn't seem from some quick testing that we could run into a
situation where net->ifname could be NULL in that second call - one
would have to set bandwidth options... in any case to keep Coverity
happy and perhaps be extra paranoid a "if (net->ifname &&
actualBandwidth)" would avoid the situation.
Beyond that it seems things are fine... So consider it an ACK as long as
you address the Coverity error
John
+ if (virNetDevGetIndex(net->ifname, &nicindex)
< 0 ||
+ VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex) < 0)
+ goto cleanup;
+ }
+ break;
}
case VIR_DOMAIN_NET_TYPE_USER:
@@ -8257,9 +8259,6 @@ qemuBuildCommandLine(virConnectPtr conn,
virUUIDFormat(def->uuid, uuid);
- *nnicindexes = 0;
- *nicindexes = NULL;
-
emulator = def->emulator;
if (!cfg->privileged) {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bec05d4..04fa8fa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6447,8 +6447,6 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
size_t i;
virQEMUDriverConfigPtr cfg;
virCapsPtr caps = NULL;
- size_t nnicindexes = 0;
- int *nicindexes = NULL;
virCheckFlags(0, NULL);
@@ -6634,14 +6632,12 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
&buildCommandLineCallbacks,
true,
qemuCheckFips(),
- NULL,
- &nnicindexes, &nicindexes)))
+ NULL, NULL, NULL)))
goto cleanup;
ret = virCommandToString(cmd);
cleanup:
- VIR_FREE(nicindexes);
virObjectUnref(qemuCaps);
virCommandFree(cmd);
virDomainDefFree(def);
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 16f325e..7eba5c9 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -263,8 +263,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
char *log = NULL;
virCommandPtr cmd = NULL;
size_t i;
- size_t nnicindexes = 0;
- int *nicindexes = NULL;
virBitmapPtr nodeset = NULL;
if (!(conn = virGetConnect()))
@@ -355,7 +353,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
&testCallbacks, false,
(flags & FLAG_FIPS),
- nodeset, &nnicindexes, &nicindexes))) {
+ nodeset, NULL, NULL))) {
if (!virtTestOOMActive() &&
(flags & FLAG_EXPECT_FAILURE)) {
ret = 0;
@@ -402,7 +400,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
ret = 0;
out:
- VIR_FREE(nicindexes);
VIR_FREE(log);
VIR_FREE(expectargv);
VIR_FREE(actualargv);
diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c
index a068135..4220737 100644
--- a/tests/qemuxmlnstest.c
+++ b/tests/qemuxmlnstest.c
@@ -44,8 +44,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
char *log = NULL;
char *emulator = NULL;
virCommandPtr cmd = NULL;
- size_t nnicindexes = 0;
- int *nicindexes = NULL;
if (!(conn = virGetConnect()))
goto fail;
@@ -122,7 +120,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
migrateFrom, migrateFd, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
&testCallbacks, false, false, NULL,
- &nnicindexes, &nicindexes)))
+ NULL, NULL)))
goto fail;
if (!virtTestOOMActive()) {
@@ -158,7 +156,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
ret = 0;
fail:
- VIR_FREE(nicindexes);
VIR_FREE(log);
VIR_FREE(emulator);
VIR_FREE(expectargv);