[libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests

[Andrea Bolognani]

On Mon, 2018-09-17 at 14:10 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午11:21, Andrea Bolognani 写道:
> > > @@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
> > >       if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
> > >           goto exit_monitor;
> > >   
> > > -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> > > +    if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
> > > +        goto exit_monitor;
> > > +
> > 
> > Since the zpci device refers to the underlying device through the
> > target= option, does it make sense to attach the zpci device first
> > and the target device second? I would expect it to fail unless you
> > attach them the other way around...
> 
> This attaching order is right. Qemu requires that attach zpci first and 
> target device second.
> In Qemu, zpci is created firstly and ready to build connection with 
> target device, and after
> target device is plugged, there's a mapping built for zpci and target 
> device. If the order is
> reversed, there would be error.

Cool, let's leave it as it is then.

> > [...]
> > > @@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
> > >           goto cleanup;
> > >   
> > >       qemuDomainObjEnterMonitor(driver, vm);
> > > -    ret = qemuMonitorAddDevice(priv->mon, devstr);
> > > +
> > > +    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
> > > +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> > > +        goto cleanup;
> > > +    }
> > 
> > I'm not sure this is entirely safe. Perhaps you should introduce an
> > exit_monitor label that ensures failure to exit the monitor is also
> > taken into account, and jump to that one instead?
> 
> I see there're a lot of code ignoring monitor exit failure. Of course, 
> taking it into account
> is proper. Do you strongly suggest to add that?

As mentioned a while ago, I'm not particularly comfortable around
this part of libvirt, so we should definitely have someone with
more experience look it over before merging.

That said, while there are indeed a few existing places where the
return value of qemuDomainObjExitMonitor() is ignored, it seems
like it's far more common to take it into account, so I would say
do that unless you have a very good reason not to.

> > [...]
> > > +    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
> > > +                                          configfd, configfd_name)) < 0)
> > > +        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
> > 
> > Parentheses around here, please.
> 
> ok.

(Of course I meant curly braces, not parentheses :)

-- 
Andrea Bolognani / Red Hat / Virtualization