[libvirt] [PATCH v4 1/9] conf: Add new domain XML element 'iothreadids'

[Peter Krempa]

On Tue, Apr 21, 2015 at 19:31:22 -0400, John Ferlan wrote:
> Adding a new XML element 'iothreadids' in order to allow defining
> specific IOThread ID's rather than relying on the algorithm to assign
> IOThread ID's starting at 1 and incrementing to iothreads count.
> 
> This will allow future patches to be able to add new IOThreads by
> a specific iothread_id and of course delete any exisiting IOThread.
> 
> Each iothreadids element will have 'n' <iothread> children elements
> which will have attribute "id".  The "id" will allow for definition
> of any "valid" (eg > 0) iothread_id value.
> 
> On input, if any <iothreadids> <iothread>'s are provided, they will
> be marked so that we only print out what we read in.
> 
> On input, if no <iothreadids> are provided, the PostParse code will
> self generate a list of ID's starting at 1 and going to the number
> of iothreads defined for the domain (just like the current algorithm
> numbering scheme).  A future patch will rework the existing algorithm
> to make use of the iothreadids list.
> 
> On output, only print out the <iothreadids> if they were read in.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  docs/formatdomain.html.in     |  30 +++++++
>  docs/schemas/domaincommon.rng |  12 +++
>  src/conf/domain_conf.c        | 200 +++++++++++++++++++++++++++++++++++++++++-
>  src/conf/domain_conf.h        |  18 ++++
>  src/libvirt_private.syms      |   4 +
>  5 files changed, 262 insertions(+), 2 deletions(-)
> 

...

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 479b4c2..da1bb7e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2114,6 +2114,31 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
>      return NULL;
>  }
>  
> +
> +void
> +virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def)
> +{
> +    if (def)
> +        VIR_FREE(def);

This breaks syntax check.

> +}
> +
> +
> +static void
> +virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def,
> +                                int nids)
> +{
> +    size_t i;
> +
> +    if (!def)
> +        return;
> +
> +    for (i = 0; i < nids; i++)
> +        virDomainIOThreadIDDefFree(def[i]);
> +
> +    VIR_FREE(def);
> +}
> +
> +
>  void
>  virDomainPinDefFree(virDomainPinDefPtr def)
>  {

....

> @@ -3298,6 +3325,21 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>          return -1;
>      }
>  
> +    /* Fully populate the IOThread ID list */
> +    if (def->iothreads && def->iothreads != def->niothreadids) {
> +        unsigned int iothread_id = 1;
> +        while (def->niothreadids != def->iothreads) {
> +            if (!virDomainIOThreadIDFind(def, iothread_id)) {
> +                virDomainIOThreadIDDefPtr iothrid;
> +
> +                if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id)))
> +                    return -1;

Unfortunately, fixing the iothread list after you parse iothread pinning
in patch 4 makes a loophole where you might force arbitrary iothread ID
without using <iothreadids>.

This code will probably need to be moved after the parsing code, despite
the fact that the postparse callback is better place to do such checks.

> +                iothrid->autofill = true;
> +            }
> +            iothread_id++;
> +        }
> +    }
> +
>      if (virDomainDefGetMemoryInitial(def) == 0) {
>          virReportError(VIR_ERR_XML_ERROR, "%s",
>                         _("Memory size must be specified via <memory> or in the "

The rest of this patch looks good, but I'd like to see the above part
fixed before my final ACK.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150423/2fc3370c/attachment-0001.sig>