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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...] > > 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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;
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.