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.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list