[libvirt] [PATCH 0/3] Couple of <interface/> improvements
https://bugzilla.redhat.com/show_bug.cgi?id=1343919 Michal Privoznik (3): qemuParseCommandLineNet: Make more readable virmacaddr: Track if MAC address is autogenerated virDomainNetFindIdx: Ignore auto generated MAC addresses src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++---------- src/qemu/qemu_parse_command.c | 32 ++++++++++++------------------ src/util/virmacaddr.c | 5 +++++ src/util/virmacaddr.h | 2 ++ tests/bhyveargv2xmlmock.c | 1 + tests/bhyvexml2argvmock.c | 1 + 6 files changed, 56 insertions(+), 30 deletions(-) -- 2.13.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_parse_command.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index c9112dd90..37e1149c0 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -1055,9 +1055,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, if (virStrToLong_i(values[i], NULL, 10, &wantvlan) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse vlan in '%s'"), val); - virDomainNetDefFree(def); - def = NULL; - goto cleanup; + goto error; } } else if (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET && STREQ(keywords[i], "script") && STRNEQ(values[i], "")) { @@ -1076,18 +1074,13 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, */ nic = qemuFindNICForVLAN(nnics, nics, wantvlan); - if (!nic) { - virDomainNetDefFree(def); - def = NULL; - goto cleanup; - } + if (!nic) + goto error; if (!STRPREFIX(nic, "nic")) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse NIC definition '%s'"), nic); - virDomainNetDefFree(def); - def = NULL; - goto cleanup; + goto error; } for (i = 0; i < nkeywords; i++) { @@ -1103,9 +1096,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, &values, &nkeywords, 0) < 0) { - virDomainNetDefFree(def); - def = NULL; - goto cleanup; + goto error; } } else { nkeywords = 0; @@ -1118,9 +1109,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse mac address '%s'"), values[i]); - virDomainNetDefFree(def); - def = NULL; - goto cleanup; + goto error; } } else if (STREQ(keywords[i], "model")) { def->model = values[i]; @@ -1135,9 +1124,7 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, if (virStrToLong_ul(values[i], NULL, 10, &def->tune.sndbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse sndbuf size in '%s'"), val); - virDomainNetDefFree(def); - def = NULL; - goto cleanup; + goto error; } def->tune.sndbuf_specified = true; } @@ -1154,6 +1141,11 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, VIR_FREE(keywords); VIR_FREE(values); return def; + + error: + virDomainNetDefFree(def); + def = NULL; + goto cleanup; } -- 2.13.5
On Mon, Oct 02, 2017 at 01:01:18PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_parse_command.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-)
Reviewed-by: Erik Skultety <eskultet@redhat.com>
I also think 'Make *it* more readable' sounds a bit better. Erik
It will come handy to know if the MAC address was generated (e.g. during XML parse) or if it was parsed since provided by user in the XML. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virmacaddr.c | 5 +++++ src/util/virmacaddr.h | 2 ++ tests/bhyveargv2xmlmock.c | 1 + tests/bhyvexml2argvmock.c | 1 + 4 files changed, 9 insertions(+) diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c index 7afe032b9..409fdc34d 100644 --- a/src/util/virmacaddr.c +++ b/src/util/virmacaddr.c @@ -107,6 +107,7 @@ void virMacAddrSet(virMacAddrPtr dst, const virMacAddr *src) { memcpy(dst, src, sizeof(*src)); + dst->generated = false; } /** @@ -120,6 +121,7 @@ void virMacAddrSetRaw(virMacAddrPtr dst, const unsigned char src[VIR_MAC_BUFLEN]) { memcpy(dst->addr, src, VIR_MAC_BUFLEN); + dst->generated = false; } /** @@ -149,6 +151,7 @@ virMacAddrParse(const char* str, virMacAddrPtr addr) { size_t i; + addr->generated = false; errno = 0; for (i = 0; i < VIR_MAC_BUFLEN; i++) { char *end_ptr; @@ -217,6 +220,7 @@ virMacAddrParseHex(const char *str, virMacAddrPtr addr) str[VIR_MAC_HEXLEN]) return -1; + addr->generated = false; for (i = 0; i < VIR_MAC_BUFLEN; i++) addr->addr[i] = (virHexToBin(str[2 * i]) << 4 | virHexToBin(str[2 * i + 1])); @@ -232,6 +236,7 @@ void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN], addr->addr[3] = virRandomBits(8); addr->addr[4] = virRandomBits(8); addr->addr[5] = virRandomBits(8); + addr->generated = true; } /* The low order bit of the first byte is the "multicast" bit. */ diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h index f4f5e2ce1..ef4285d63 100644 --- a/src/util/virmacaddr.h +++ b/src/util/virmacaddr.h @@ -36,6 +36,8 @@ typedef virMacAddr *virMacAddrPtr; struct _virMacAddr { unsigned char addr[VIR_MAC_BUFLEN]; + bool generated; /* True if MAC address was autogenerated, + false otherwise. */ }; int virMacAddrCompare(const char *mac1, const char *mac2); diff --git a/tests/bhyveargv2xmlmock.c b/tests/bhyveargv2xmlmock.c index 1f08bebb7..dd25f4e13 100644 --- a/tests/bhyveargv2xmlmock.c +++ b/tests/bhyveargv2xmlmock.c @@ -16,6 +16,7 @@ virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN], addr->addr[3] = 0; addr->addr[4] = 0; addr->addr[5] = 0; + addr->generated = true; } int diff --git a/tests/bhyvexml2argvmock.c b/tests/bhyvexml2argvmock.c index 7afa0e34c..bec7f902a 100644 --- a/tests/bhyvexml2argvmock.c +++ b/tests/bhyvexml2argvmock.c @@ -17,6 +17,7 @@ void virMacAddrGenerate(const unsigned char prefix[VIR_MAC_PREFIX_BUFLEN], addr->addr[3] = 0; addr->addr[4] = 0; addr->addr[5] = 0; + addr->generated = true; } int virNetDevTapCreateInBridgePort(const char *brname ATTRIBUTE_UNUSED, -- 2.13.5
On Mon, Oct 02, 2017 at 01:01:19PM +0200, Michal Privoznik wrote:
It will come handy to know if the MAC address was generated (e.g. during XML parse) or if it was parsed since provided by user in the XML.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We definitely should track whether the MAC was or wasn't generated. Reviewed-by: Erik Skultety <eskultet@redhat.com>
When detaching an <interface/> from domain, it's MAC address is parsed and if not present one is generated. If, however, no corresponding interface is found in the domain, the following error is reported: error: operation failed: no device matching mac address 52:54:00:75:32:5b found where the MAC address is the auto generated one. This might be very confusing. Solution to this is to ignore auto generated MAC address when looking up the device. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 87192eb2d..aab43d307 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15634,11 +15634,17 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) return 0; } -/* virDomainNetFindIdx: search according to mac address and guest side - * PCI address (if specified) +/** + * virDomainNetFindIdx: + * @def: domain definition + * @net: interface definition * - * Return: index of match if unique match found - * -1 otherwise and an error is logged + * Lookup domain's network interface based on passed @net + * definition. If @net's MAC address was auto generated, + * the MAC comparison is ignored. + * + * Return: index of match if unique match found, + * -1 otherwise and an error is logged. */ int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) @@ -15646,11 +15652,13 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) size_t i; int matchidx = -1; char mac[VIR_MAC_STRING_BUFLEN]; + bool MACAddrSpecified = !net->mac.generated; bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI); for (i = 0; i < def->nnets; i++) { - if (virMacAddrCmp(&def->nets[i]->mac, &net->mac)) + if (MACAddrSpecified && + virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0) continue; if ((matchidx >= 0) && !PCIAddrSpecified) { @@ -15660,9 +15668,15 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) * specify only vendor and product ID, and there may be * multiples of those. */ - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple devices matching mac address %s found"), - virMacAddrFormat(&net->mac, mac)); + if (MACAddrSpecified) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple devices matching mac address %s found"), + virMacAddrFormat(&net->mac, mac)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("multiple matching devices found")); + } + return -1; } if (PCIAddrSpecified) { @@ -15679,8 +15693,9 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) matchidx = i; } } + if (matchidx < 0) { - if (PCIAddrSpecified) { + if (MACAddrSpecified && PCIAddrSpecified) { virReportError(VIR_ERR_OPERATION_FAILED, _("no device matching mac address %s found on " "%.4x:%.2x:%.2x.%.1x"), @@ -15689,10 +15704,20 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) net->info.addr.pci.bus, net->info.addr.pci.slot, net->info.addr.pci.function); - } else { + } else if (PCIAddrSpecified) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("no device found on %.4x:%.2x:%.2x.%.1x"), + net->info.addr.pci.domain, + net->info.addr.pci.bus, + net->info.addr.pci.slot, + net->info.addr.pci.function); + } else if (MACAddrSpecified) { virReportError(VIR_ERR_OPERATION_FAILED, _("no device matching mac address %s found"), virMacAddrFormat(&net->mac, mac)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("no matching device found")); } } return matchidx; -- 2.13.5
On Mon, Oct 02, 2017 at 01:01:20PM +0200, Michal Privoznik wrote:
> When detaching an <interface/> from domain, it's MAC address is
> parsed and if not present one is generated. If, however, no
> corresponding interface is found in the domain, the following
> error is reported:
>
> error: operation failed: no device matching mac address 52:54:00:75:32:5b found
>
> where the MAC address is the auto generated one. This might be
> very confusing. Solution to this is to ignore auto generated MAC
> address when looking up the device.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
While I think that a better fix would be to not generate the MAC during the
parsing stage because it's semantically wrong, I also understand that changing
that properly wouldn't be possible without a large refactor. Also, I'm not
experienced enough in the networking code to be so sure that moving the burden
of generating the MAC to individual drivers rather than doing it in the parser
is a viable solution for all the drivers. Therefore I agree with this approach,
it gets the job done, but if someone has a stronger opinion backed by solid
arguments as for why it should be worth to refactor the whole thing, speak up.
> src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 87192eb2d..aab43d307 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15634,11 +15634,17 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
> return 0;
> }
>
> -/* virDomainNetFindIdx: search according to mac address and guest side
> - * PCI address (if specified)
> +/**
> + * virDomainNetFindIdx:
> + * @def: domain definition
> + * @net: interface definition
> *
> - * Return: index of match if unique match found
> - * -1 otherwise and an error is logged
> + * Lookup domain's network interface based on passed @net
> + * definition. If @net's MAC address was auto generated,
> + * the MAC comparison is ignored.
> + *
> + * Return: index of match if unique match found,
> + * -1 otherwise and an error is logged.
> */
> int
> virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> @@ -15646,11 +15652,13 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> size_t i;
> int matchidx = -1;
> char mac[VIR_MAC_STRING_BUFLEN];
> + bool MACAddrSpecified = !net->mac.generated;
> bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info,
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
>
> for (i = 0; i < def->nnets; i++) {
> - if (virMacAddrCmp(&def->nets[i]->mac, &net->mac))
> + if (MACAddrSpecified &&
> + virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
> continue;
Code-wise, I would probably enhance this loop in the following manner:
- move the PCI lookup code above ^this hunk because semantically, PCI match has
a precedence over MAC (probably because we allow multiple devices with the
same MAC, but there might be other historical reasons for that), so you save
some CPU cycles
>
> if ((matchidx >= 0) && !PCIAddrSpecified) {
- you could then drop the second operand ^here, because PCI would be handled
first (and it breaks the loop), so we get here only with MAC lookup
> @@ -15660,9 +15668,15 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> * specify only vendor and product ID, and there may be
> * multiples of those.
> */
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching mac address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> + if (MACAddrSpecified) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("multiple devices matching mac address %s found"),
> + virMacAddrFormat(&net->mac, mac));
> + } else {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("multiple matching devices found"));
> + }
> +
> return -1;
> }
> if (PCIAddrSpecified) {
- ^this block would be moved for the reasons described above
> @@ -15679,8 +15693,9 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> matchidx = i;
- and ^this assignment could be left unguarded by any if-else statement
Eventually, a patch preceding yours could be similar to the following (I didn't
bother rebasing and making the changes, I made them on top of this patch, I'm
sure you'll get the picture):
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aab43d307..db0451f45 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15657,11 +15657,23 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
for (i = 0; i < def->nnets; i++) {
+ if (PCIAddrSpecified) {
+ if (!virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
+ &net->info.addr.pci))
+ continue;
+
+ /* exit early if the pci address was specified and
+ * it matches, as this guarantees no duplicates.
+ */
+ matchidx = i;
+ break;
+ }
+
if (MACAddrSpecified &&
virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
continue;
- if ((matchidx >= 0) && !PCIAddrSpecified) {
+ if (matchidx >= 0) {
/* there were multiple matches on mac address, and no
* qualifying guest-side PCI address was given, so we must
* fail (NB: a USB address isn't adequate, since it may
@@ -15678,20 +15690,11 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
}
return -1;
+
}
- if (PCIAddrSpecified) {
- if (virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
- &net->info.addr.pci)) {
- /* exit early if the pci address was specified and
- * it matches, as this guarantees no duplicates.
- */
- matchidx = i;
- break;
- }
- } else {
- /* no PCI address given, so there may be multiple matches */
- matchidx = i;
- }
+
+ /* no PCI address given, so there may be multiple matches */
+ matchidx = i;
}
if (matchidx < 0) {
Anyhow, you have my
Reviewed-by: Erik Skultety <eskultet@redhat.com> to this patch.
On 10/04/2017 01:19 PM, Erik Skultety wrote:
> On Mon, Oct 02, 2017 at 01:01:20PM +0200, Michal Privoznik wrote:
>> When detaching an <interface/> from domain, it's MAC address is
>> parsed and if not present one is generated. If, however, no
>> corresponding interface is found in the domain, the following
>> error is reported:
>>
>> error: operation failed: no device matching mac address 52:54:00:75:32:5b found
>>
>> where the MAC address is the auto generated one. This might be
>> very confusing. Solution to this is to ignore auto generated MAC
>> address when looking up the device.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>
> While I think that a better fix would be to not generate the MAC during the
> parsing stage because it's semantically wrong, I also understand that changing
> that properly wouldn't be possible without a large refactor. Also, I'm not
> experienced enough in the networking code to be so sure that moving the burden
> of generating the MAC to individual drivers rather than doing it in the parser
> is a viable solution for all the drivers. Therefore I agree with this approach,
> it gets the job done, but if someone has a stronger opinion backed by solid
> arguments as for why it should be worth to refactor the whole thing, speak up.
>
>> src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 35 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 87192eb2d..aab43d307 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -15634,11 +15634,17 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
>> return 0;
>> }
>>
>> -/* virDomainNetFindIdx: search according to mac address and guest side
>> - * PCI address (if specified)
>> +/**
>> + * virDomainNetFindIdx:
>> + * @def: domain definition
>> + * @net: interface definition
>> *
>> - * Return: index of match if unique match found
>> - * -1 otherwise and an error is logged
>> + * Lookup domain's network interface based on passed @net
>> + * definition. If @net's MAC address was auto generated,
>> + * the MAC comparison is ignored.
>> + *
>> + * Return: index of match if unique match found,
>> + * -1 otherwise and an error is logged.
>> */
>> int
>> virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>> @@ -15646,11 +15652,13 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>> size_t i;
>> int matchidx = -1;
>> char mac[VIR_MAC_STRING_BUFLEN];
>> + bool MACAddrSpecified = !net->mac.generated;
>> bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info,
>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
>>
>> for (i = 0; i < def->nnets; i++) {
>> - if (virMacAddrCmp(&def->nets[i]->mac, &net->mac))
>> + if (MACAddrSpecified &&
>> + virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
>> continue;
>
> Code-wise, I would probably enhance this loop in the following manner:
> - move the PCI lookup code above ^this hunk because semantically, PCI match has
> a precedence over MAC (probably because we allow multiple devices with the
> same MAC, but there might be other historical reasons for that), so you save
> some CPU cycles
>
>>
>> if ((matchidx >= 0) && !PCIAddrSpecified) {
>
> - you could then drop the second operand ^here, because PCI would be handled
> first (and it breaks the loop), so we get here only with MAC lookup
>
>> @@ -15660,9 +15668,15 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>> * specify only vendor and product ID, and there may be
>> * multiples of those.
>> */
>> - virReportError(VIR_ERR_OPERATION_FAILED,
>> - _("multiple devices matching mac address %s found"),
>> - virMacAddrFormat(&net->mac, mac));
>> + if (MACAddrSpecified) {
>> + virReportError(VIR_ERR_OPERATION_FAILED,
>> + _("multiple devices matching mac address %s found"),
>> + virMacAddrFormat(&net->mac, mac));
>> + } else {
>> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> + _("multiple matching devices found"));
>> + }
>> +
>> return -1;
>> }
>> if (PCIAddrSpecified) {
> - ^this block would be moved for the reasons described above
>
>> @@ -15679,8 +15693,9 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>> matchidx = i;
>
> - and ^this assignment could be left unguarded by any if-else statement
>
> Eventually, a patch preceding yours could be similar to the following (I didn't
> bother rebasing and making the changes, I made them on top of this patch, I'm
> sure you'll get the picture):
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index aab43d307..db0451f45 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15657,11 +15657,23 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
>
> for (i = 0; i < def->nnets; i++) {
> + if (PCIAddrSpecified) {
> + if (!virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
> + &net->info.addr.pci))
> + continue;
> +
> + /* exit early if the pci address was specified and
> + * it matches, as this guarantees no duplicates.
> + */
> + matchidx = i;
> + break;
> + }
I don't think this is right. Consider the following scenario. There are
two interfaces in a domain with $mac1, $pci1 and $mac2, $pci2. What
happens if user passes $mac1+$pci2 or vice versa? With my code, we error
out. With this suggestion we match device based on PCI address (even
though MAC address doesn't match). But I agree that we can do better
here, so I'm squashing this in:
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index a9523df6a..34993de73 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -15676,7 +15676,12 @@ virDomainNetFindIdx(virDomainDefPtr def,
virDomainNetDefPtr net)
virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
continue;
- if ((matchidx >= 0) && !PCIAddrSpecified) {
+ if (PCIAddrSpecified &&
+ !virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
+ &net->info.addr.pci))
+ continue;
+
+ if (matchidx >= 0) {
/* there were multiple matches on mac address, and no
* qualifying guest-side PCI address was given, so we must
* fail (NB: a USB address isn't adequate, since it may
@@ -15694,19 +15699,8 @@ virDomainNetFindIdx(virDomainDefPtr def,
virDomainNetDefPtr net)
return -1;
}
- if (PCIAddrSpecified) {
- if (virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
- &net->info.addr.pci)) {
- /* exit early if the pci address was specified and
- * it matches, as this guarantees no duplicates.
- */
- matchidx = i;
- break;
- }
- } else {
- /* no PCI address given, so there may be multiple matches */
- matchidx = i;
- }
+
+ matchidx = i;
}
if (matchidx < 0) {
>
> Anyhow, you have my
> Reviewed-by: Erik Skultety <eskultet@redhat.com> to this patch.
>
Thanks.
Michal
[...]
I don't think this is right. Consider the following scenario. There are two interfaces in a domain with $mac1, $pci1 and $mac2, $pci2. What happens if user passes $mac1+$pci2 or vice versa? With my code, we error out. With this suggestion we match device based on PCI address (even
Fair point, I haven't considered this scenario, as I couldn't imagine someone hitting this corner case. Nevertheless, doesn't matter, mine assumption was wrong, thank you for pointing it out. Erik
On 10/02/2017 07:01 AM, Michal Privoznik wrote:
When detaching an <interface/> from domain, it's MAC address is
s/from domain/from a domain/ s/it's/the
parsed and if not present one is generated. If, however, no
s/, however,//
corresponding interface is found in the domain, the following error is reported:
error: operation failed: no device matching mac address 52:54:00:75:32:5b found
where the MAC address is the auto generated one. This might be very confusing. Solution to this is to ignore auto generated MAC address when looking up the device.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-)
I had looked at this series too, but forgot to send this when something else caused me to reboot.... Patch 1 and 2 I had no comments... My primary comment here is should the new message generated in some way indicate that it's the generated mac... It's no big deal, but just a thought and may help compared to having to look for the generic message.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 87192eb2d..aab43d307 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15634,11 +15634,17 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) return 0; }
-/* virDomainNetFindIdx: search according to mac address and guest side - * PCI address (if specified) +/** + * virDomainNetFindIdx: + * @def: domain definition + * @net: interface definition * - * Return: index of match if unique match found - * -1 otherwise and an error is logged + * Lookup domain's network interface based on passed @net + * definition. If @net's MAC address was auto generated, + * the MAC comparison is ignored. + * + * Return: index of match if unique match found, + * -1 otherwise and an error is logged. */ int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) @@ -15646,11 +15652,13 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) size_t i; int matchidx = -1; char mac[VIR_MAC_STRING_BUFLEN]; + bool MACAddrSpecified = !net->mac.generated; bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
for (i = 0; i < def->nnets; i++) { - if (virMacAddrCmp(&def->nets[i]->mac, &net->mac)) + if (MACAddrSpecified && + virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0) continue;
if ((matchidx >= 0) && !PCIAddrSpecified) { @@ -15660,9 +15668,15 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) * specify only vendor and product ID, and there may be * multiples of those. */ - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple devices matching mac address %s found"), - virMacAddrFormat(&net->mac, mac)); + if (MACAddrSpecified) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple devices matching mac address %s found"), + virMacAddrFormat(&net->mac, mac)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("multiple matching devices found"));
multiple devices matching generated mac address found ?
+ } + return -1; } if (PCIAddrSpecified) { @@ -15679,8 +15693,9 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) matchidx = i; } } + if (matchidx < 0) { - if (PCIAddrSpecified) { + if (MACAddrSpecified && PCIAddrSpecified) { virReportError(VIR_ERR_OPERATION_FAILED, _("no device matching mac address %s found on " "%.4x:%.2x:%.2x.%.1x"), @@ -15689,10 +15704,20 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) net->info.addr.pci.bus, net->info.addr.pci.slot, net->info.addr.pci.function); - } else { + } else if (PCIAddrSpecified) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("no device found on %.4x:%.2x:%.2x.%.1x"),
no device using generated mac address found on ... ?
+ net->info.addr.pci.domain, + net->info.addr.pci.bus, + net->info.addr.pci.slot, + net->info.addr.pci.function); + } else if (MACAddrSpecified) { virReportError(VIR_ERR_OPERATION_FAILED, _("no device matching mac address %s found"), virMacAddrFormat(&net->mac, mac)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("no matching device found"));
no device using generated mac address found ?
} } return matchidx;
On 10/04/2017 03:28 PM, John Ferlan wrote:
On 10/02/2017 07:01 AM, Michal Privoznik wrote:
When detaching an <interface/> from domain, it's MAC address is
s/from domain/from a domain/ s/it's/the
parsed and if not present one is generated. If, however, no
s/, however,//
corresponding interface is found in the domain, the following error is reported:
error: operation failed: no device matching mac address 52:54:00:75:32:5b found
where the MAC address is the auto generated one. This might be very confusing. Solution to this is to ignore auto generated MAC address when looking up the device.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-)
I had looked at this series too, but forgot to send this when something else caused me to reboot.... Patch 1 and 2 I had no comments...
My primary comment here is should the new message generated in some way indicate that it's the generated mac... It's no big deal, but just a thought and may help compared to having to look for the generic message.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 87192eb2d..aab43d307 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15634,11 +15634,17 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) return 0; }
-/* virDomainNetFindIdx: search according to mac address and guest side - * PCI address (if specified) +/** + * virDomainNetFindIdx: + * @def: domain definition + * @net: interface definition * - * Return: index of match if unique match found - * -1 otherwise and an error is logged + * Lookup domain's network interface based on passed @net + * definition. If @net's MAC address was auto generated, + * the MAC comparison is ignored. + * + * Return: index of match if unique match found, + * -1 otherwise and an error is logged. */ int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) @@ -15646,11 +15652,13 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) size_t i; int matchidx = -1; char mac[VIR_MAC_STRING_BUFLEN]; + bool MACAddrSpecified = !net->mac.generated; bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
for (i = 0; i < def->nnets; i++) { - if (virMacAddrCmp(&def->nets[i]->mac, &net->mac)) + if (MACAddrSpecified && + virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0) continue;
if ((matchidx >= 0) && !PCIAddrSpecified) { @@ -15660,9 +15668,15 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net) * specify only vendor and product ID, and there may be * multiples of those. */ - virReportError(VIR_ERR_OPERATION_FAILED, - _("multiple devices matching mac address %s found"), - virMacAddrFormat(&net->mac, mac)); + if (MACAddrSpecified) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple devices matching mac address %s found"), + virMacAddrFormat(&net->mac, mac)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("multiple matching devices found"));
multiple devices matching generated mac address found ?
I think this is misleading. The whole point of this patch is that if the MAC was generated whilst parsing user's XML, it is not taken into consideration. Therefore, other unique attributes are consulted - so far it's just PCI address. Yes, the MAC address is still generated, but not taken into consideration when matching. That's why I'd like to avoid printing it out (it's confusing anyway), and hence this piece of code. Michal
participants (3)
-
Erik Skultety -
John Ferlan -
Michal Privoznik