On 04/01/2014 06:11 PM, Ján Tomko wrote:
> Every caller checked the return value and logged an error
> - one if no device with the specified MAC was found,
> other if there were multiple devices matching the MAC address
> (except for qemuDomainUpdateDeviceConfig which logged the same
> message in both cases).
>
> Move the error reporting into virDomainNetFindIdx, since in both cases,
> we couldn't find one single match - it's just the error messages that
> differ.
Well, when I originally put in the check for duplicate MAC addresses and
the differing return, I guess I had anticipated that
virDomainNetFindIdx() would be used in other cases where multiple
matches wouldn't be an error (e.g. when adding a new interface). However
it's been a year and a half and still every call (even the 3 new calls
from lxc code) treats multiple matches as an error, just like no match.
So definitely this makes sense, since it consolidates the error
reporting to a single place which makes the error messages consistent.
ACK.
(And if we ever do need the find function to return -2 for multiple
matches in the future, we can always add that back in with a flag,
retaining the consolidated error messages).
> ---
> src/conf/domain_conf.c | 15 +++++++++++----
> src/lxc/lxc_driver.c | 39 +++++----------------------------------
> src/qemu/qemu_driver.c | 28 +++-------------------------
> src/qemu/qemu_hotplug.c | 15 ++-------------
> 4 files changed, 21 insertions(+), 76 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6fb216e..1624c7e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10414,14 +10414,14 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
> * PCI address (if specified)
> *
> * Return: index of match if unique match found
> - * -1 if not found
> - * -2 if multiple matches
> + * -1 otherwise and an error is logged
> */
> int
> virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> {
> size_t i;
> int matchidx = -1;
> + char mac[VIR_MAC_STRING_BUFLEN];
> bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info,
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
>
> @@ -10436,8 +10436,10 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> * specify only vendor and product ID, and there may be
> * multiples of those.
> */
> - matchidx = -2; /* indicates "multiple matches" to caller */
> - break;
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("multiple devices matching mac address %s found"),
> + virMacAddrFormat(&net->mac, mac));
> + return -1;
> }
> if (PCIAddrSpecified) {
> if (virDevicePCIAddressEqual(&def->nets[i]->info.addr.pci,
> @@ -10453,6 +10455,11 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> matchidx = i;
> }
> }
> + if (matchidx < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("no device matching mac address %s found"),
> + virMacAddrFormat(&net->mac, mac));
> + }
> return matchidx;
> }
>
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 05464cb..b900bc6 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -3768,22 +3768,12 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
> int ret = -1;
> virDomainNetDefPtr net;
> int idx;
> - char mac[VIR_MAC_STRING_BUFLEN];
>
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_NET:
> net = dev->data.net;
> - idx = virDomainNetFindIdx(vmdef, net);
> - if (idx == -2) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching mac address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> - goto cleanup;
> - } else if (idx < 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("no matching network device was found"));
> + if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
> goto cleanup;
> - }
>
> virDomainNetDefFree(vmdef->nets[idx]);
>
> @@ -3813,7 +3803,6 @@ lxcDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> virDomainNetDefPtr net;
> virDomainHostdevDefPtr hostdev, det_hostdev;
> int idx;
> - char mac[VIR_MAC_STRING_BUFLEN];
>
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
> @@ -3829,17 +3818,9 @@ lxcDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>
> case VIR_DOMAIN_DEVICE_NET:
> net = dev->data.net;
> - idx = virDomainNetFindIdx(vmdef, net);
> - if (idx == -2) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching mac address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> - goto cleanup;
> - } else if (idx < 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("no matching network device was found"));
> + if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
> goto cleanup;
> - }
> +
> /* this is guaranteed to succeed */
> virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
> ret = 0;
> @@ -4650,21 +4631,11 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
> {
> int detachidx, ret = -1;
> virDomainNetDefPtr detach = NULL;
> - char mac[VIR_MAC_STRING_BUFLEN];
> virNetDevVPortProfilePtr vport = NULL;
>
> - detachidx = virDomainNetFindIdx(vm->def, dev->data.net);
> - if (detachidx == -2) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching mac address %s found"),
> - virMacAddrFormat(&dev->data.net->mac, mac));
> - goto cleanup;
> - } else if (detachidx < 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("network device %s not found"),
> - virMacAddrFormat(&dev->data.net->mac, mac));
> + if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
> goto cleanup;
> - }
> +
> detach = vm->def->nets[detachidx];
>
> switch (virDomainNetGetActualType(detach)) {
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b032441..384f430 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6763,7 +6763,6 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> virDomainChrDefPtr chr;
> virDomainFSDefPtr fs;
> int idx;
> - char mac[VIR_MAC_STRING_BUFLEN];
>
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
> @@ -6778,17 +6777,9 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>
> case VIR_DOMAIN_DEVICE_NET:
> net = dev->data.net;
> - idx = virDomainNetFindIdx(vmdef, net);
> - if (idx == -2) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching mac address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> + if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
> return -1;
> - } else if (idx < 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("no matching network device was found"));
> - return -1;
> - }
> +
> /* this is guaranteed to succeed */
> virDomainNetDefFree(virDomainNetRemove(vmdef, idx));
> break;
> @@ -6868,7 +6859,6 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
> virDomainDiskDefPtr orig, disk;
> virDomainNetDefPtr net;
> int pos;
> - char mac[VIR_MAC_STRING_BUFLEN];
>
>
> switch (dev->type) {
> @@ -6907,20 +6897,8 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
>
> case VIR_DOMAIN_DEVICE_NET:
> net = dev->data.net;
> - pos = virDomainNetFindIdx(vmdef, net);
> - if (pos == -2) {
> - virMacAddrFormat(&net->mac, mac);
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("couldn't find matching device "
> - "with mac address %s"), mac);
> - return -1;
> - } else if (pos < 0) {
> - virMacAddrFormat(&net->mac, mac);
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("couldn't find matching device "
> - "with mac address %s"), mac);
> + if ((pos = virDomainNetFindIdx(vmdef, net)) < 0)
> return -1;
> - }
>
> virDomainNetDefFree(vmdef->nets[pos]);
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 19d96cb..20a75bc 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3362,21 +3362,10 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int vlan;
> char *hostnet_name = NULL;
> - char mac[VIR_MAC_STRING_BUFLEN];
>
> - detachidx = virDomainNetFindIdx(vm->def, dev->data.net);
> - if (detachidx == -2) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching mac address %s found"),
> - virMacAddrFormat(&dev->data.net->mac, mac));
> - goto cleanup;
> - }
> - else if (detachidx < 0) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("network device %s not found"),
> - virMacAddrFormat(&dev->data.net->mac, mac));
> + if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
> goto cleanup;
> - }
> +
> detach = vm->def->nets[detachidx];
>
> if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list