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

Re: [libvirt] [PATCHv2 4/5] Add virDomain{S, G}etInterfaceParameters support to qemu driver



On Wed, Dec 28, 2011 at 03:23:39PM +0800, Osier Yang wrote:
> On 2011年12月23日 15:09, Hu Tao wrote:
> >* src/qemu/qemu_driver.c: implement the qemu driver support
> >---
> >  src/qemu/qemu_driver.c |  434 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 434 insertions(+), 0 deletions(-)
> >
> >diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >index c908135..2fab489 100644
> >--- a/src/qemu/qemu_driver.c
> >+++ b/src/qemu/qemu_driver.c
> >@@ -119,6 +119,8 @@
> >
> >  #define QEMU_NB_BLKIO_PARAM  2
> >
> >+#define QEMU_NB_BANDWIDTH_PARAM 6
> >+
> >  static void processWatchdogEvent(void *data, void *opaque);
> >
> >  static int qemudShutdown(void);
> >@@ -7846,6 +7848,436 @@ qemudDomainInterfaceStats (virDomainPtr dom,
> >  #endif
> >
> >  static int
> >+qemuDomainSetInterfaceParameters(virDomainPtr dom,
> >+                                 const char *device,
> >+                                 virTypedParameterPtr params,
> >+                                 int nparams,
> >+                                 unsigned int flags)
> >+{
> >+    struct qemud_driver *driver = dom->conn->privateData;
> >+    int i;
> >+    virCgroupPtr group = NULL;
> >+    virDomainObjPtr vm = NULL;
> >+    virDomainDefPtr persistentDef = NULL;
> >+    int ret = -1;
> >+    virDomainNetDefPtr net = NULL;
> >+    bool isMac = false;
> >+    virNetDevBandwidthPtr bandwidth;
> >+    unsigned char mac[VIR_MAC_BUFLEN];
> >+    bool isActive;
> >+
> >+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> >+                  VIR_DOMAIN_AFFECT_CONFIG, -1);
> >+    qemuDriverLock(driver);
> >+
> >+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> >+
> >+    if (vm == NULL) {
> >+        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                        _("No such domain %s"), dom->uuid);
> >+        goto cleanup;
> >+    }
> 
> == From
> >+
> >+    isActive = virDomainObjIsActive(vm);
> >+
> >+    if (flags == VIR_DOMAIN_AFFECT_CURRENT) {
> >+        if (isActive)
> >+            flags = VIR_DOMAIN_AFFECT_LIVE;
> >+        else
> >+            flags = VIR_DOMAIN_AFFECT_CONFIG;
> >+    }
> >+
> >+    if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
> >+        if (!isActive) {
> >+            qemuReportError(VIR_ERR_OPERATION_INVALID,
> >+                            "%s", _("domain is not running"));
> >+            goto cleanup;
> >+        }
> >+    }
> >+
> >+    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
> >+        if (!vm->persistent) {
> >+            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> >+                            _("cannot change persistent config of a transient domain"));
> >+            goto cleanup;
> >+        }
> >+        if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
> >+            goto cleanup;
> >+    }
> 
> == To
> 
> The above block can be shortened with using helper function
> virDomainLiveConfigHelperMethod. See commit ae52342.
> 
> >+
> >+    if (VIR_ALLOC(bandwidth)<  0) {
> >+        virReportOOMError();
> >+        goto cleanup;
> >+    }
> >+    if (VIR_ALLOC(bandwidth->in)<  0) {
> >+        virReportOOMError();
> >+        goto cleanup;
> >+    }
> >+    memset(bandwidth->in, 0, sizeof(*bandwidth->in));
> >+    if (VIR_ALLOC(bandwidth->out)<  0) {
> >+        virReportOOMError();
> >+        goto cleanup;
> >+    }
> >+    memset(bandwidth->out, 0, sizeof(*bandwidth->out));
> >+
> >+    for (i = 0; i<  nparams; i++) {
> >+        virTypedParameterPtr param =&params[i];
> >+
> >+        if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_IN_AVERAGE)) {
> >+            if (param->type != VIR_TYPED_PARAM_UINT) {
> >+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> >+                                _("invalid type for bandwidth average tunable, expected a 'unsigned int'"));
> >+                goto cleanup;
> >+            }
> >+
> >+            bandwidth->in->average = params[i].value.ui;
> >+        } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_IN_PEAK)) {
> >+            if (param->type != VIR_TYPED_PARAM_UINT) {
> >+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> >+                                _("invalid type for bandwidth peak tunable, expected a 'unsigned int'"));
> >+                goto cleanup;
> >+            }
> >+
> >+            bandwidth->in->peak = params[i].value.ui;
> >+        } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_IN_BURST)) {
> >+            if (param->type != VIR_TYPED_PARAM_UINT) {
> >+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> >+                                _("invalid type for bandwidth burst tunable, expected a 'unsigned int'"));
> >+                goto cleanup;
> >+            }
> >+
> >+            bandwidth->in->burst = params[i].value.ui;
> >+        } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE)) {
> >+            if (param->type != VIR_TYPED_PARAM_UINT) {
> >+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> >+                                _("invalid type for bandwidth average tunable, expected a 'unsigned int'"));
> >+                goto cleanup;
> >+            }
> >+
> >+            bandwidth->out->average = params[i].value.ui;
> >+        } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_OUT_PEAK)) {
> >+            if (param->type != VIR_TYPED_PARAM_UINT) {
> >+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> >+                                _("invalid type for bandwidth peak tunable, expected a 'unsigned int'"));
> >+                goto cleanup;
> >+            }
> >+
> >+            bandwidth->out->peak = params[i].value.ui;
> >+        } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_OUT_BURST)) {
> >+            if (param->type != VIR_TYPED_PARAM_UINT) {
> >+                qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> >+                                _("invalid type for bandwidth burst tunable, expected a 'unsigned int'"));
> >+                goto cleanup;
> >+            }
> >+
> >+            bandwidth->out->burst = params[i].value.ui;
> >+        } else {
> >+            qemuReportError(VIR_ERR_INVALID_ARG,
> >+                            _("Parameter `%s' not supported"),
> >+                            param->field);
> >+            goto cleanup;
> >+        }
> >+    }
> >+
> >+    /* average is mandatory, peak and burst is optional. So if no
> >+     * average is given, we free inbound/outbound here which causes
> >+     * inbound/outbound won't be set. */
> >+    if (!bandwidth->in->average)
> >+        VIR_FREE(bandwidth->in);
> >+    if (!bandwidth->out->average)
> >+        VIR_FREE(bandwidth->out);
> >+
> >+    if (virParseMacAddr(device, mac) == 0)
> >+        isMac = true;
> >+
> >+    ret = 0;
> >+    if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
> >+        if (isMac) {
> >+            for (i = 0; i<  vm->def->nnets; i++) {
> >+                if (memcmp(mac, vm->def->nets[i]->mac, VIR_MAC_BUFLEN) == 0) {
> >+                    net = vm->def->nets[i];
> >+                    break;
> >+                }
> >+            }
> >+        } else { /* ifname */
> >+            for (i = 0; i<  vm->def->nnets; i++) {
> >+                if (STREQ(device, vm->def->nets[i]->ifname)) {
> >+                    net = vm->def->nets[i];
> >+                    break;
> >+                }
> >+            }
> >+        }
> 
> Should we have a helper function to find the net device? accepting
> both MAC and ifname. Per it's used twice in this function, and
> once in qemuDomainGetInterfaceParameters. And also it's very likely
> be used in future.
> 
> >+        if (!net) {
> >+            qemuReportError(VIR_ERR_INVALID_ARG,
> >+                            _("cannt find device %s"),
> >+                            device);
> >+            goto cleanup;
> >+        }
> 
> And it's better to quit ealier before allocating memory for
> bandwidth and parsing params if the device can't be found, it's
> just waste to do those work if the device even doesn't exist.
> 
> >+        if (virNetDevBandwidthSet(net->ifname, bandwidth)<  0) {
> >+            qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                            _("cannot set bandwidth limits on %s"),
> >+                            device);
> >+            ret = -1;
> 
> Is this intended? to me it's wrong to change the net config below,
> since it bandwidth setting already failed. "goto cleanup;" makes
> sense.
> 
> >+        }
> >+
> >+        if (!net->bandwidth) {
> >+            net->bandwidth = bandwidth;
> >+            bandwidth = NULL;
> >+        } else {
> >+            if (bandwidth->in) {
> >+                VIR_FREE(net->bandwidth->in);
> >+                net->bandwidth->in = bandwidth->in;
> >+                bandwidth->in = NULL;
> >+            }
> >+            if (bandwidth->out) {
> >+                VIR_FREE(net->bandwidth->out);
> >+                net->bandwidth->out = bandwidth->out;
> >+                bandwidth->out = NULL;
> >+            }
> >+        }
> >+    }
> >+    if (ret<  0)
> >+        goto cleanup;
> 
> Again, failed on setting the bandwidth, but net config is changed.
> 
> 
> >+    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
> >+        if (isMac) {
> >+            for (i = 0; i<  persistentDef->nnets; i++) {
> >+                if (memcmp(mac, persistentDef->nets[i]->mac, VIR_MAC_BUFLEN) == 0) {
> >+                    net = persistentDef->nets[i];
> >+                    break;
> >+                }
> >+            }
> >+        } else { /* ifname */
> >+            for (i = 0; i<  persistentDef->nnets; i++) {
> >+                if (STREQ(device, persistentDef->nets[i]->ifname)) {
> >+                    net = persistentDef->nets[i];
> >+                    break;
> >+                }
> >+            }
> >+        }
> >+        if (!net) {
> >+            qemuReportError(VIR_ERR_INVALID_ARG,
> >+                            _("Can't find device %s"), device);
> >+            ret = -1;
> >+            goto cleanup;
> >+        }
> >+
> >+        if (!net->bandwidth) {
> >+            net->bandwidth = bandwidth;
> >+            bandwidth = NULL;
> >+        } else {
> >+            if (bandwidth->in) {
> >+                VIR_FREE(net->bandwidth->in);
> >+                net->bandwidth->in = bandwidth->in;
> >+                bandwidth->in = NULL;
> >+            }
> >+            if (bandwidth->out) {
> >+                VIR_FREE(net->bandwidth->out);
> >+                net->bandwidth->out = bandwidth->out;
> >+                bandwidth->out = NULL;
> >+            }
> >+        }
> >+
> >+        if (virDomainSaveConfig(driver->configDir, persistentDef)<  0)
> >+            ret = -1;
> >+    }
> >+
> >+cleanup:
> >+    if (bandwidth) {
> >+        VIR_FREE(bandwidth->in);
> >+        VIR_FREE(bandwidth->out);
> >+        VIR_FREE(bandwidth);
> >+    }
> >+    virCgroupFree(&group);
> >+    if (vm)
> >+        virDomainObjUnlock(vm);
> >+    qemuDriverUnlock(driver);
> >+    return ret;
> >+}
> >+
> >+static int
> >+qemuDomainGetInterfaceParameters(virDomainPtr dom,
> >+                                 const char *device,
> >+                                 virTypedParameterPtr params,
> >+                                 int *nparams,
> >+                                 unsigned int flags)
> >+{
> >+    struct qemud_driver *driver = dom->conn->privateData;
> >+    int i;
> >+    virCgroupPtr group = NULL;
> >+    virDomainObjPtr vm = NULL;
> >+    virDomainDefPtr def = NULL;
> >+    virDomainNetDefPtr net = NULL;
> >+    bool isMac = false;
> >+    unsigned char mac[VIR_MAC_BUFLEN];
> >+    int ret = -1;
> >+    bool isActive;
> >+
> >+    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> >+                  VIR_DOMAIN_AFFECT_CONFIG |
> >+                  VIR_TYPED_PARAM_STRING_OKAY, -1);
> >+
> >+    qemuDriverLock(driver);
> >+
> >+    flags&= ~VIR_TYPED_PARAM_STRING_OKAY;
> >+
> >+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> >+
> >+    if (vm == NULL) {
> >+        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                        _("No such domain %s"), dom->uuid);
> >+        goto cleanup;
> >+    }
> >+
> >+    isActive = virDomainObjIsActive(vm);
> >+
> >+    if (flags == VIR_DOMAIN_AFFECT_CURRENT) {
> >+        if (isActive)
> >+            flags = VIR_DOMAIN_AFFECT_LIVE;
> >+        else
> >+            flags = VIR_DOMAIN_AFFECT_CONFIG;
> >+    }
> >+
> >+    if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
> >+        if (!isActive) {
> >+            qemuReportError(VIR_ERR_OPERATION_INVALID,
> >+                            "%s", _("domain is not running"));
> >+            goto cleanup;
> >+        }
> >+        def = vm->def;
> >+    }
> >+
> >+    if (flags&  VIR_DOMAIN_AFFECT_CONFIG) {
> >+        if (!vm->persistent) {
> >+            qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> >+                            _("cannot change persistent config of a transient domain"));
> >+            goto cleanup;
> >+        }
> >+        if (!(def = virDomainObjGetPersistentDef(driver->caps, vm)))
> >+            goto cleanup;
> >+    }
> 
> Likewise,
> 
> >+
> >+    if ((*nparams) == 0) {
> >+        *nparams = QEMU_NB_BANDWIDTH_PARAM;
> >+        ret = 0;
> >+        goto cleanup;
> >+    }
> >+
> >+    if ((*nparams)<  QEMU_NB_BANDWIDTH_PARAM) {
> >+        qemuReportError(VIR_ERR_INVALID_ARG,
> >+                        "%s", _("Invalid parameter count"));
> >+        goto cleanup;
> >+    }
> 
> Should we force *nparams >= QEMU_NB_BANDWIDTH_PARAM? IMO it's
> not neccessary.
> 
> >+
> >+    if (virParseMacAddr(device, mac) == 0)
> >+        isMac = true;
> >+
> >+    if (isMac) {
> >+        for (i = 0; i<  def->nnets; i++) {
> >+            if (memcmp(mac, def->nets[i]->mac, VIR_MAC_BUFLEN) == 0) {
> >+                net = def->nets[i];
> >+                break;
> >+            }
> >+        }
> >+    } else { /* ifname */
> >+        for (i = 0; i<  def->nnets; i++) {
> >+            if (STREQ(device, def->nets[i]->ifname)) {
> >+                net = def->nets[i];
> >+                break;
> >+            }
> >+        }
> >+    }
> >+
> >+    if (!net) {
> >+        qemuReportError(VIR_ERR_INVALID_ARG,
> >+                        _("Can't find device %s"), device);
> >+        ret = -1;
> >+        goto cleanup;
> >+    }
> >+
> >+    for (i = 0; i<  *nparams&&  i<  QEMU_NB_BANDWIDTH_PARAM; i++) {
> >+        params[i].value.ui = 0;
> >+        params[i].type = VIR_TYPED_PARAM_UINT;
> >+
> >+        switch(i) {
> >+        case 0: /* inbound.average */
> >+            if (virStrcpyStatic(params[i].field, VIR_DOMAIN_BANDWIDTH_IN_AVERAGE) == NULL) {
> >+                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                                _("Field name '%s' too long"),
> >+                                VIR_DOMAIN_BANDWIDTH_IN_AVERAGE);
> >+                goto cleanup;
> >+            }
> >+            if (net->bandwidth&&  net->bandwidth->in)
> >+                params[i].value.ui = net->bandwidth->in->average;
> >+            break;
> >+        case 1: /* inbound.peak */
> >+            if (virStrcpyStatic(params[i].field, VIR_DOMAIN_BANDWIDTH_IN_PEAK) == NULL) {
> >+                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                                _("Field name '%s' too long"),
> >+                                VIR_DOMAIN_BANDWIDTH_IN_PEAK);
> >+                goto cleanup;
> >+            }
> >+            if (net->bandwidth&&  net->bandwidth->in)
> >+                params[i].value.ui = net->bandwidth->in->peak;
> >+            break;
> >+        case 2: /* inbound.burst */
> >+            if (virStrcpyStatic(params[i].field, VIR_DOMAIN_BANDWIDTH_IN_BURST) == NULL) {
> >+                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                                _("Field name '%s' too long"),
> >+                                VIR_DOMAIN_BANDWIDTH_IN_BURST);
> >+                goto cleanup;
> >+            }
> >+            if (net->bandwidth&&  net->bandwidth->in)
> >+                params[i].value.ui = net->bandwidth->in->burst;
> >+            break;
> >+        case 3: /* outbound.average */
> >+            if (virStrcpyStatic(params[i].field, VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE) == NULL) {
> >+                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                                _("Field name '%s' too long"),
> >+                                VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE);
> >+                goto cleanup;
> >+            }
> >+            if (net->bandwidth&&  net->bandwidth->out)
> >+                params[i].value.ui = net->bandwidth->out->average;
> >+            break;
> >+        case 4: /* outbound.peak */
> >+            if (virStrcpyStatic(params[i].field, VIR_DOMAIN_BANDWIDTH_OUT_PEAK) == NULL) {
> >+                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                                _("Field name '%s' too long"),
> >+                                VIR_DOMAIN_BANDWIDTH_OUT_PEAK);
> >+                goto cleanup;
> >+            }
> >+            if (net->bandwidth&&  net->bandwidth->out)
> >+                params[i].value.ui = net->bandwidth->out->peak;
> >+            break;
> >+        case 5: /* outbound.burst */
> >+            if (virStrcpyStatic(params[i].field, VIR_DOMAIN_BANDWIDTH_OUT_BURST) == NULL) {
> >+                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> >+                                _("Field name '%s' too long"),
> >+                                VIR_DOMAIN_BANDWIDTH_OUT_BURST);
> >+                goto cleanup;
> >+            }
> >+            if (net->bandwidth&&  net->bandwidth->out)
> >+                params[i].value.ui = net->bandwidth->out->burst;
> >+            break;
> >+        default:
> >+            break;
> >+            /* should not hit here */
> >+        }
> >+    }
> >+
> >+    *nparams = QEMU_NB_BANDWIDTH_PARAM;
> 
> If we allow *nprams < QEMU_NB_BANDWIDTH_PARAM, this should be updated.
> 
> >+    ret = 0;
> >+
> >+cleanup:
> >+    if (group)
> >+        virCgroupFree(&group);
> >+    if (vm)
> >+        virDomainObjUnlock(vm);
> >+    qemuDriverUnlock(driver);
> >+    return ret;
> >+}
> >+
> >+static int
> >  qemudDomainMemoryStats (virDomainPtr dom,
> >                          struct _virDomainMemoryStat *stats,
> >                          unsigned int nr_stats,
> >@@ -11636,6 +12068,8 @@ static virDriver qemuDriver = {
> >      .domainGetBlockIoTune = qemuDomainGetBlockIoTune, /* 0.9.8 */
> >      .domainSetNumaParameters = qemuDomainSetNumaParameters, /* 0.9.9 */
> >      .domainGetNumaParameters = qemuDomainGetNumaParameters, /* 0.9.9 */
> >+    .domainGetInterfaceParameters = qemuDomainGetInterfaceParameters, /* 0.9.9 */
> >+    .domainSetInterfaceParameters = qemuDomainSetInterfaceParameters, /* 0.9.9 */
> >  };
> >
> >
> 
> Others looks fine.

Thanks for your review, I posted a v3 to address the problems you
pointed out.

-- 
Thanks,
Hu Tao


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