Re: [libvirt] [PATCH 1/8] Add new API virDomain{Set, Get}BlockIoTune

On 11/14/2011 09:33 PM, Lei Li wrote: > This patch add new pulic API virDomainSetBlockIoTune and > virDomainGetBlockIoTune. > > +/** > + * virDomainSetBlockIoTune: > + * @dom: pointer to domain object > + * @disk: Fully-qualified disk name Hmm. Fully-qualified name here, > + * @params: Pointer to blkio parameter objects > + * @nparams: Number of blkio parameters (this value can be the same or > + * less than the number of parameters supported) > + * @flags: An OR'ed set of virDomainModificationImpact > + * > + * Change all or a subset of the per-device block IO tunables. > + * > + * The @disk parameter is the name of the block device. Get this > + * by calling virDomainGetXMLDesc and finding the<target dev='...'> > + * attribute within //domain/devices/disk. (For example, "xvda"). but device shorthand here. We probably ought to accept both forms, just as we do with virDomainBlockStats, as of commit 89b6284f (oh, and that means that I ought to update virDomainBlockStats to document the same thing) - which means this can be a separate cleanup to make libvirt.c docs consistent as well as teaching the new API to use the domain_conf.c virDomainDiskIndexByName method to allow both forms. I can get to that after finishing my review of this series, if you don't beat me.

> + * Get all block IO tunable parameters for a given device. On input, > + * @nparams gives the size of the @params array; on output, @nparams > + * gives how many slots were filled with parameter information, which > + * might be less but will not exceed the input value. > + * > + * As a special case, calling with @params as NULL and @nparams as 0 on > + * input will cause @nparams on output to contain the number of parameters > + * supported by the hypervisor. The caller should then allocate @params > + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API > + * agiain. Trailing space (running 'make syntax-check' will gripe).

s/agiain/again/ Also, probably worth mentioning that the value of @nparams may be disk-specific, so the user is best off reprobing an appropriate nparams for each disk rather than assuming the parameters from 1 disk apply to all others.

> +++ b/src/libvirt_public.syms > @@ -496,6 +496,8 @@ LIBVIRT_0.9.7 { > virDomainSnapshotGetParent; > virDomainSnapshotListChildrenNames; > virDomainSnapshotNumChildren; > + virDomainSetBlockIoTune; > + virDomainGetBlockIoTune; > } LIBVIRT_0.9.5; This needs to be in a new section LIBVIRT_0.9.8 based off of LIBVIRT_0.9.5 (we aren't changing 0.9.7 at this point). The problems I mentioned are minor enough that I don't mind fixing them prior to pushing, if the rest of the series looks sane; but if later review finds more problems later in the series, we may need a v5 series instead. I'll see about reviewing the rest of the series this week (I've also got quite a few nwfilter patches backed up in my todo queue).

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list