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 =¶ms[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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list