[libvirt] [PATCHv2] qemu: Validate address type when attaching a disk device.

[Ruifeng Bian]

----- Original Message -----
> From: "Ján Tomko" <jtomko at redhat.com>
> To: "Ruifeng Bian" <rbian at redhat.com>
> Cc: libvir-list at redhat.com
> Sent: Tuesday, September 8, 2015 8:07:21 PM
> Subject: Re: [libvirt] [PATCHv2] qemu: Validate address type when attaching a disk device.
> 
> On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1257844
> > 
> > Attach-device can hotplug a virtio disk device with any address type now,
> > it need to validate the address type before the attachment.
> > 
> > Coldplug a scsi disk device without checking the address type in current
> > version, this patch also fix the scsi disk problem.
> > ---
> >  src/qemu/qemu_driver.c  |  8 ++++++++
> >  src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 91eb661..af926fc 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr
> > qemuCaps,
> >          if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
> >              if (virDomainDefAddImplicitControllers(vmdef) < 0)
> >                  return -1;
> > +        /* scsi disk should have an address type of driver */
> > +        if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
> > +            (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) {
> > +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                               _("scsi disk cannot have an address of type
> > '%s'"),
> > +
> > virDomainDeviceAddressTypeToString(disk->info.type));
> > +            return -1;
> > +        }
> 
> This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the
> check for DISK_BUS_VIRTIO.
> 
> Should we check other bus types as well?

Yes, other bus types still have problem, I will collect all types.
> 
> I think the most important part of the bug is that we allow hotplugging
> a device with broken configuration, then fail to unplug it because it is
> broken. Here detach-device works with --config even with an incorrect
> address type.

Detach-device should also check address types, I will add it.

> 
> >          if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> >              return -1;
> >          break;
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 63fafa6..4226650 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
> >          if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info,
> >          priv->qemuCaps,
> >                                              disk->dst))
> >              goto cleanup;
> > +
> > +        /* virtio device should either have a ccw or pci address */
> > +        if (qemuDomainMachineIsS390CCW(vm->def) &&
> > +            virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
> > +            if (!virDomainDeviceAddressIsValid(&disk->info,
> > +
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW))
> > {
> > +                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > +                               _("device cannot be attached without a
> > valid CCW address"));
> > +                goto cleanup;
> > +            }
> 
> Is hotplugging a device with a S390 type address impossible on a s390-ccw
> machine?
> 
> > +        } else {
> > +            if (!virDomainDeviceAddressIsValid(&disk->info,
> > +
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI))
> > {
> > +                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > +                               _("device cannot be attached without a
> > valid PCI address"));
> 
> These are not the only two options. At this point, the address type can
> also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE
> (and a PCI address will be generated by virDomainPCIAddressEnsureAddr).

ADDRESS_TYPE_NONE won't come here, since disk->info.type is 0.

> 
> The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For
> ancient QEMUs which did not support -device, we use the PCI address to
> uniquely identify the device. In that case, detaching a virtio disk with
> no address is not possible.
> 
> For all reasonable versions of QEMU, the device alias is used, so
> detaching devices with other address types is possible.
> But we explicitly require a PCI or CCW address, to make sure we will
> unplug only the device requested by user. S390 seems to be missing here,
> but that might be on purpose.
> 
> Allowing S390 addresses on hotplug without allowing them on hotunplug
> won't fix the linked bug for S390 machines, but attaching a device with
> no PCI address is a valid use case - libvirt will just generate one.
> 

Yes, the check code from qemuDomainDetachVirtioDiskDevice. I thought the code should
work both for attach and detach. 

If user provide virtio device address for attachment, the address type should be PCI,
CCW or S390. We can check machine type to see if CCW address should be used. But how to
check S390 address should be used? Could I use virQEMUCapsGet(priv->qemuCaps, 
QEMU_CAPS_VIRTIO_S390)? In all other cases, PCI address should be used. Right?


> > +                goto cleaup;
> 
> Please make sure 'make check' and 'make syntax-check' pass before
> sending a patch to the list, as described on our hacking page:
> http://libvirt.org/hacking.html
> 
> Jan
>