Re: [libvirt] [PATCH] qemu: fix attach/detach of netdevs with matching mac addrs
On 10/26/2012 04:54 PM, Eric Blake wrote:
On 10/25/2012 02:31 PM, Laine Stump wrote:
> This resolves:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=862515
>
> which describes inconsistencies in dealing with duplicate mac
> addresses on network devices in a domain.
>
> (at any rate, it resolves *almost* everything, and prints out an
> informative error message for the one problem that isn't solved, but
> has a workaround.)
>
> @@ -6158,14 +6158,6 @@ qemuDomainAttachDeviceConfig(qemuCapsPtr caps,
>
> case VIR_DOMAIN_DEVICE_NET:
> net = dev->data.net;
> - if (virDomainNetIndexByMac(vmdef, &net->mac) >= 0) {
> - char macbuf[VIR_MAC_STRING_BUFLEN];
> -
> - virMacAddrFormat(&net->mac, macbuf);
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("mac %s already exists"), macbuf);
> - return -1;
> - }
> if (virDomainNetInsert(vmdef, net)) {
Are you dropping functionality here? Shouldn't you still be checking
that the new NetFindIdx function returns -1?
No, that was intentional, and noted in the log message. It is incorrect
to prohibit duplicate mac addresses for persistent (and live) attach.
We've never checked for it in the live attach code, and we should check
for it here.
> - vshError(ctl, _("No found interface whose MAC address is %s"), mac);
> - goto cleanup;
> + if (!matchNode) {
> + vshError(ctl, _("No found interface whose MAC address is %s"),
mac);
This would read better as 'No interface found', since you are already
touching this line.
I changed it to "No interface with MAC address %s was found"
ACK with those nits addressed; I like the overall improvement in error
messages.
Pushed. Thanks!