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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list