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

[Peter Krempa]

On Mon, Apr 27, 2015 at 11:56:20 -0400, John Ferlan wrote:
> 
> 
> On 04/27/2015 11:46 AM, Peter Krempa wrote:
> > On Mon, Apr 27, 2015 at 11:25:15 -0400, John Ferlan wrote:
> >> ...
> >>
> >>>>>> --- 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.
> > 
> > Agreed, this should be done separately.
> > 
> >>
> >> Other issues were addressed changed - do you need to see the diffs or an
> >> updated patch with the diffs already squashed in?
> > 
> > I'd like to see the fixed hunk of qemuProcessDetectIOThreadPIDs that
> > parses the reply from the monitor.
> > 
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 66c9321..3def84f 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2249,13 +2249,18 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver,
>  
>      for (i = 0; i < vm->def->niothreadids; i++) {

The code should iterate through niothreads rather than
vm->def->niothreadids for obvious reasons even if they are guaranteed
the same.

>          unsigned int iothread_id;
> +        virDomainIOThreadIDDefPtr iothrid;
>  
>          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;
> +        if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("iothread %d not found"), iothread_id);
> +            goto cleanup;
> +        }
> +        iothrid->thread_id = iothreads[i]->thread_id;
>      }
>  
>      ret = 0;
> 

ACK with the suggested modification.
-------------- 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/20150427/83e94483/attachment-0001.sig>