Re: [PATCH 1/2] Move error reporting into virDomainNetFindIdx

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]