12:29 p.m.
On 12/09/2015 09:01 AM, John Ferlan wrote:
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
You left a thought half-fin
:-)
> ---
> 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" ?
Okay. Done.
> +{
> + 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...
The difference is that for the network device we were doing an entire
netlink message/response cycle for each index that we tried, which was
*very* slow. So we made a data structure in memory that we could more
easily scan. In this case we are already working with data in memory
that we can easily scan, so I think there would be very little to gain.
(note that in the case of tap devices, the kernel will already
automatically create a unique name for us, so we don't need to try and
guess as we do for macvtap devices).
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?
No, I think the people who added macvtap were just trying to make
something that worked and figured they'd come back and optimize it
later, but then forgot the 2nd half.
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.
Since everything is just looking through the entries of an array in
memory comparing integers, I don't see the problem with leaving it like
this even for a few hundred controllers, and I think we will have fallen
to the machine overlords before we have to deal with a single virtual
machine with more controllers than that.
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++;
}
You're assuming that the items in the list are in order of index.
Although there is a function that does that when inserting new elements
(virDomainControllerInsertPreAlloced()) I don't have complete confidence
that it is always used to add new controllers. And besides I think the
counts are low enough that a simple function will do fine (and be easier
to understand - it took me a few minutes to parse yours and understand
its assumptions).
(oh, as a matter of fact, virDomainControllerInsertPreAlloced() *isn't*
always called - virDomainDefMaybeAddController() didn't use it, and
after this patch virDomainControllerDefAddController() doesn't (because
it just used the existing code in the other function).
}
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 ;-)
Yeah. I'll fix that.
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...
Okay. I changed the name and eliminated the extra superfluous else.