[libvirt] [PATCH v5 02/10] qemu: Use domain iothreadids to IOThread's 'thread_id'

[John Ferlan]

...

>>>> --- a/src/qemu/qemu_process.c
>>>> +++ b/src/qemu/qemu_process.c
>>>> @@ -2267,12 +2267,16 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
>>>>          goto cleanup;
>>>>      }
>>>
>>> A few lines prior here is the check that the expected thread count
>>> equals to the actual thread count. For some reason a few lines before
>>> returns success if 0 threads are returned by the monitor. The two checks
>>> should be inverted so that it makes sense.
>>>
>>
>> If there are no threads, then it's not a failure, thus change ret to be
>> 0. Again, this is something that's not within the scope of this set of
>> changes and I believe if necessary could be a followup patch.
>>
>> I'm not clear on the value of changing the order of the checks.
> 
> The problem is that if there are no iothreads reported by qemu, but we
> did request some then it IS failure.
> 

But that's an issue not related to iothreadid's per se - it's a more
common general issue that should be a follow-up patch then I think.
That is not introduced by this set of changes.

Other issues were addressed changed - do you need to see the diffs or an
updated patch with the diffs already squashed in?

John

>>
>> Check failure first - return failure
>>
>> Check possible successes next.
>>
>>>>  
>>>> -    if (VIR_ALLOC_N(priv->iothreadpids, niothreads) < 0)
>>>> -        goto cleanup;
>>>> -    priv->niothreadpids = niothreads;
>>>> +    for (i = 0; i < vm->def->niothreadids; i++) {
>>>> +        unsigned int iothread_id;
>>>>  
>>>> -    for (i = 0; i < priv->niothreadpids; i++)
>>>> -        priv->iothreadpids[i] = iothreads[i]->thread_id;
>>>> +        if (qemuDomainParseIOThreadAlias(iothreads[i]->name,
>>>> +                                         &iothread_id) < 0)
>>>> +            goto cleanup;
>>>> +
>>>> +        vm->def->iothreadids[i]->thread_id = iothreads[i]->thread_id;
>>>> +        vm->def->iothreadids[i]->iothread_id = iothread_id;
>>>
>>> This construct is wrong since it expects that the order the devices are
>>> stored in libvirt is the same as in the qemu monitor reply. You need to
>>> iterate the list of threads from the monitor and lookup the
>>> corresponding info for it.
>>
>> Ahh... Right, but we are getting the iothread_id's here from the monitor
>> and filling in an array - the call to virDomainIOThreadIDFind had better
>> not fail ;-) - if does though the domain disappears, so which is worse?
>>
>> So ...
>>
>>     virDomainIOThreadIDDefPtr iothrid;
>> ...
>>
>>     if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) {
>>         virReportError(VIR_ERR_INVALID_ARG,
>>                        _("iothread %d not found"), iothread_id);
>>     }
>>     iothrid->thread_id = iothreads[i]->thread_id;
> 
> Yep.
> 
>>
>>>
>>> Btw, it would be much easier if the monitor code would parse the IDs
>>> since you wouldn't need to parse them here (I've already suggested this
>>> once).
>>>
>>
>> Again, design/structure change - can we let this be a followup patch?
> 
> Yes this can be a separate change. I only find it less wasteful since
> every single caller needs to parse the IDs anyways.
> 
>>
>>>> +    }
>>>>  
>>>>      ret = 0;
>>>>  
>>>
>>> ...
>>>
> 
> Peter
>