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

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.