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

Re: [libvirt] [PATCH v2 11/14] libxl: support network device attach/detach



On 06/12/2013 07:54 PM, Marek Marczykowski-Górecki wrote:
Both live and config.

Signed-off-by: Marek Marczykowski-Górecki <marmarek invisiblethingslab com>
---
  src/libxl/libxl_driver.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 161 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 7b50853..ae0d4f7 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3417,6 +3417,111 @@ cleanup:
  }
static int
+libxlDomainAttachDeviceNetLive(libxlDriverPrivatePtr driver,
+                               libxlDomainObjPrivatePtr priv,
+                               virDomainObjPtr vm,
+                               virDomainDeviceDefPtr dev)
+{
+    virDomainNetDefPtr l_net = dev->data.net;
+    libxl_device_nic x_nic;
+    char mac[VIR_MAC_STRING_BUFLEN];
+    int ret = -1;
+
+    switch (dev->data.net->type)  {

l_net->type ?

+        case VIR_DOMAIN_NET_TYPE_ETHERNET:
+        case VIR_DOMAIN_NET_TYPE_BRIDGE:
+            /* -2 means "multiple matches" so then fail also */
+            if (virDomainNetFindIdx(vm->def, dev->data.net) != -1) {

And just pass l_net here. Also, you should check for -2. I think this is better coded as

        idx = virDomainNetFindIdx(vm->def, l_net);
        if (idx == -2) {
            virReportError(VIR_ERR_OPERATION_FAILED,
                           _("multiple devices matching mac address %s found"),
                           virMacAddrFormat(&net->mac, mac));
            return -1;
        } else if (idx < 0) {
            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
                           _("no matching network device was found"));
            return -1;
        }


+                virReportError(VIR_ERR_OPERATION_FAILED,
+                        _("device matching mac address %s already exists"),

We should use the same error message as the qemu driver, which is included in my snippet above.

+                        virMacAddrFormat(&dev->data.net->mac, mac));
+                goto cleanup;
+            }
+
+            if (libxlMakeNic(driver, l_net, &x_nic) < 0)
+                goto cleanup;
+
+            if ((ret = libxl_device_nic_add(priv->ctx, vm->def->id,
+                            &x_nic, NULL)) < 0) {

That indentation looks a little odd.  Perhaps best to split into 2 lines

ret = libxl_deivce_nic_add(...);
if (ret < 0)

+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("libxenlight failed to attach net '%s'"),
+                        virMacAddrFormat(&dev->data.net->mac, mac));
+                goto cleanup;
+            }
+
+            virDomainNetInsert(vm->def, l_net);
+
+            break;
+        default:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                    _("net device type '%s' cannot be hotplugged"),

s/net/network/

+                    virDomainNetTypeToString(l_net->type));
+            break;
+    }
+
+cleanup:
+    return ret;

Nothing to cleanup. Might as well just return errors where they occur or success when done.

+}
+
+static int
+libxlDomainDetachDeviceNetLive(libxlDriverPrivatePtr driver,
+                               libxlDomainObjPrivatePtr priv,
+                               virDomainObjPtr vm,
+                               virDomainDeviceDefPtr dev)
+{
+    virDomainNetDefPtr l_net = NULL;
+    libxl_device_nic x_nic;
+    char mac[VIR_MAC_STRING_BUFLEN];
+    int i;
+    int ret = -1;
+
+    switch (dev->data.net->type)  {
+        case VIR_DOMAIN_NET_TYPE_ETHERNET:
+        case VIR_DOMAIN_NET_TYPE_BRIDGE:
+            if ((i = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) {
+                if (i == -2) {
+
+                    virReportError(VIR_ERR_OPERATION_FAILED,
+                            _("multiple devices matching mac address %s found"),
+                            virMacAddrFormat(&dev->data.net->mac, mac));
+                }
+                else {
+                    virReportError(VIR_ERR_OPERATION_FAILED,
+                            _("network device %s not found"),
+                            virMacAddrFormat(&dev->data.net->mac, mac));
+                }
+                goto cleanup;
+            }

For consistency, the error checking logic here should be changed to match the Attach function.

+
+            l_net = vm->def->nets[i];
+
+            if (libxlMakeNic(driver, l_net, &x_nic) < 0)
+                goto cleanup;
+
+            if ((ret = libxl_device_nic_remove(priv->ctx, vm->def->id,
+                            &x_nic, NULL)) < 0) {

Split into 2 lines?

+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("libxenlight failed to detach nic '%d'"),
+                        i);
+                goto cleanup;
+            }
+
+            virDomainNetRemove(vm->def, i);
+            virDomainNetDefFree(l_net);
+
+            break;
+        default:
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("net device type '%s' cannot hot unplugged"),

s/net/network/

+                           virDomainNetTypeToString(dev->data.net->type));
+            break;
+    }
+
+cleanup:
+    return ret;

Nothing to cleanup.

+}
+
+static int
  libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
                              libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
                              virDomainDeviceDefPtr dev)
@@ -3430,6 +3535,12 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
                  dev->data.disk = NULL;
              break;
+ case VIR_DOMAIN_DEVICE_NET:
+            ret = libxlDomainAttachDeviceNetLive(driver, priv, vm, dev);
+            if (!ret)
+                dev->data.net = NULL;
+            break;
+
          default:
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                             _("device type '%s' cannot be attached"),
@@ -3444,6 +3555,8 @@ static int
  libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
  {
      virDomainDiskDefPtr disk;
+    virDomainNetDefPtr net;
+    char mac[VIR_MAC_STRING_BUFLEN];
switch (dev->type) {
          case VIR_DOMAIN_DEVICE_DISK:
@@ -3461,6 +3574,22 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
              dev->data.disk = NULL;
              break;
+ case VIR_DOMAIN_DEVICE_NET:
+            net = dev->data.net;
+            if (virDomainNetFindIdx(vmdef, net) >= 0) {
+                virReportError(VIR_ERR_INVALID_ARG,
+                               _("net device with mac %s already exists."),

Reuse existing error message.

+                               virMacAddrFormat(&net->mac, mac));
+                return -1;
+            }
+            if (virDomainNetInsert(vmdef, net)) {
+                virReportOOMError();
+                return -1;
+            }
+            /* vmdef has the pointer. Generic codes for vmdef will do all jobs */
+            dev->data.net = NULL;
+            break;
+
          default:
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("persistent attach of device is not supported"));
@@ -3482,6 +3611,10 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver,
              ret = libxlDomainDetachDeviceDiskLive(driver, priv, vm, dev);
              break;
+ case VIR_DOMAIN_DEVICE_NET:
+            ret = libxlDomainDetachDeviceNetLive(driver, priv, vm, dev);
+            break;
+
          default:
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                             _("device type '%s' cannot be detached"),
@@ -3495,20 +3628,45 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver,
  static int
  libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
  {
-    virDomainDiskDefPtr disk, detach;
+    virDomainDiskDefPtr disk, disk_detach;
+    virDomainNetDefPtr net, net_detach;
+    char mac[VIR_MAC_STRING_BUFLEN];
+    int net_idx;
      int ret = -1;
switch (dev->type) {
          case VIR_DOMAIN_DEVICE_DISK:
              disk = dev->data.disk;
-            if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
+            if (!(disk_detach = virDomainDiskRemoveByName(vmdef, disk->dst))) {
                  virReportError(VIR_ERR_INVALID_ARG,
                                 _("no target device %s"), disk->dst);
                  break;
              }
-            virDomainDiskDefFree(detach);
+            virDomainDiskDefFree(disk_detach);
+            ret = 0;
+            break;
+
+        case VIR_DOMAIN_DEVICE_NET:
+            net = dev->data.net;
+            if ((net_idx = virDomainNetFindIdx(vmdef, net)) < 0) {
+                if (net_idx == -2) {
+
+                    virReportError(VIR_ERR_OPERATION_FAILED,
+                            _("multiple devices matching mac address %s found"),
+                            virMacAddrFormat(&dev->data.net->mac, mac));
+                }
+                else {
+                    virReportError(VIR_ERR_OPERATION_FAILED,
+                            _("network device %s not found"),
+                            virMacAddrFormat(&dev->data.net->mac, mac));
+                }
+                return -1;
+            }

Again for consistency, same error checking logic here.

Regards,
Jim

+            net_detach = virDomainNetRemove(vmdef, net_idx);
+            virDomainNetDefFree(net_detach);
              ret = 0;
              break;
+
          default:
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("persistent detach of device is not supported"));


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