[libvirt] [PATCH 3/5] conf: add virDomainDefAddController()

[John Ferlan]

On 11/19/2015 01:25 PM, Laine Stump wrote:
> We need a virDomainDefAddController() that doesn't check for an
> existing controller at the same index (since USB2 controllers must be
> added in sets of 4 that are all at the same index), so rather than
> duplicating the code in virDomainDefMaybeAddController(), split it
> into two functions, in the process eliminating existing duplicated
> code that loops through the controller list by calling
> virDomainControllerFind(), which does the same thing).

As I found while working on a different issue - the "MaybeAdd" and "Add"
functions were h
> ---
>  src/conf/domain_conf.c | 56 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0ac7dbf..ab05e7f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13456,6 +13456,18 @@ virDomainControllerFind(virDomainDefPtr def,
>  }
>  
>  
> +static int
> +virDomainControllerUnusedIndex(virDomainDefPtr def, int type)

How about "FindUnusedIndex" or "GetUnusedIndex" ?


> +{
> +    int idx = 0;
> +
> +    while (virDomainControllerFind(def, type, idx) >= 0)
> +        idx++;
> +

Something tells me virDomainControllerFind could one day be a bottleneck
as each time through we start at 0 (ncontrollers) looking for matching
'type' && 'idx'... I'm thinking of the recent head banging over the
network device (?tap*?) lookup algorithm...

One would think we don't have that many different controllers to make a
difference, but then again did we ever assume the same for the network
algorithm?  Whether creating a hash tree for each type of controller is
worth the effort would seem to be outside the scope of this set of
patches, but perhaps something that needs to be considered.

That being said - since it seems we're trying to find an available index
for a specific type of controller, perhaps it would be better to do
something like:

    idx = 0;

    for (i = 0; i < def->ncontrollers; i++) {
        if (def->controllers[i]->type == type) {
            if (def->controllers[i]->idx != idx)
                return idx;
            idx++;
        }
    }
    return idx;



> +    return idx;
> +}
> +
> +
>  const char *
>  virDomainControllerAliasFind(virDomainDefPtr def,
>                               int type, int idx)
> @@ -14375,33 +14387,45 @@ virDomainEmulatorPinDefParseXML(xmlNodePtr node)
>  }
>  
>  
> -int
> -virDomainDefMaybeAddController(virDomainDefPtr def,
> -                               int type,
> -                               int idx,
> -                               int model)
> +static virDomainControllerDefPtr
> +virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model)
>  {
> -    size_t i;
>      virDomainControllerDefPtr cont;
>  
> -    for (i = 0; i < def->ncontrollers; i++) {
> -        if (def->controllers[i]->type == type &&
> -            def->controllers[i]->idx == idx)
> -            return 0;
> -    }
> -
>      if (!(cont = virDomainControllerDefNew(type)))
> -        return -1;
> +        return NULL;
> +
> +    if (idx < 0)
> +        idx = virDomainControllerUnusedIndex(def, type);
>  
>      cont->idx = idx;
>      cont->model = model;
>  
> -    if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, cont) < 0) {
> +    if (VIR_APPEND_ELEMENT_COPY(def->controllers, def->ncontrollers, cont) < 0) {
>          VIR_FREE(cont);
> -        return -1;
> +        return NULL;
>      }
>  
> -    return 1;
> +    return cont;
> +}
> +
> +
> +int
> +virDomainDefMaybeAddController(virDomainDefPtr def,
> +                               int type,
> +                               int idx,
> +                               int model)
> +{
> +    /* skip if a specific index was given and it is already
> +     * in use for that type of controller
> +     */
> +    if (idx >= 0 && virDomainControllerFind(def, type, idx) >= 0)
> +        return 0;
> +
> +    if (virDomainDefAddController(def, type, idx, model))
> +        return 1;
> +    else
> +        return -1;

The "else" isn't necessarily necessary ;-)

ACK - I would like to see the function name change only to indicate that
it's an action Find/Get.  Whether you adjust the find algorithm depends
on whether you'd like to revisit this code some day...

John
>  }
>  
>  
>