Re: [PATCH v4] qemu: Validate address type when attaching a disk device.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



----- Original Message -----
> From: "John Ferlan" <jferlan@xxxxxxxxxx>
> To: "Ruifeng Bian" <rbian@xxxxxxxxxx>, libvir-list@xxxxxxxxxx
> Sent: Wednesday, September 30, 2015 5:31:32 AM
> Subject: Re:  [PATCH v4] qemu: Validate address type when attaching a disk device.
> 
> 
> 
> On 09/17/2015 03:35 AM, Ruifeng Bian wrote:
> > Bug fix for: 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.
> > 
> > Attaching a disk device with --config option need to check address type.
> > Otherwise, the domain cloudn't be started normally. This patch add a basic
> > check for address type.
> > 
> > Detaching a disk device with --config also need to check disk options,
> > add qemuCheckDiskConfig() method in qemuDomainDetachDeviceConfig().
> > 
> > Add virDomainDeviceAddressTypeIsValid method in domain_conf to check disk
> > address type. Address type should match with disk driver, report error
> > messages if any mismatch.
> > 
> > Signed-off-by: Ruifeng Bian <rbian@xxxxxxxxxx>
> > ---
> >  src/conf/domain_conf.c   | 32 ++++++++++++++++++++++++++++++++
> >  src/conf/domain_conf.h   |  1 +
> >  src/libvirt_private.syms |  1 +
> >  src/qemu/qemu_command.c  |  7 +++++++
> >  src/qemu/qemu_driver.c   |  2 ++
> >  src/qemu/qemu_hotplug.c  | 22 ++++++++++++++++++++++
> >  6 files changed, 65 insertions(+)
> > 
> 
> Just so this isn't left "hanging"...  Michal has generated an alternate
> method to address the issue, see:
> 
> http://www.redhat.com/archives/libvir-list/2015-September/msg00909.html
> 
> I think there could be "synergies" between the two and left some
> thoughts in the other patch which use some of this code.
> 
> Some other comments below since they may be useful...
> 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 6df1618..f86760b 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -3200,6 +3200,38 @@ int
> > virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
> >      return 0;
> >  }
> >  
> > +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
> 
> should be:
> 
> bool
> virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
> 
> > +{
> > +    if (disk->info.type) {
> > +        switch (disk->bus) {
> > +        case VIR_DOMAIN_DISK_BUS_IDE:
> > +        case VIR_DOMAIN_DISK_BUS_SCSI:
> > +        case VIR_DOMAIN_DISK_BUS_SATA:
> > +        case VIR_DOMAIN_DISK_BUS_FDC:
> > +            if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
> > +                return 1;
> 
> return false;
> 
> > +            break;
> > +        case VIR_DOMAIN_DISK_BUS_USB:
> > +            if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> > +                return 1;
> 
> return false;
> 
> > +            break;
> > +        case VIR_DOMAIN_DISK_BUS_VIRTIO:
> > +            if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> > +                disk->info.type !=
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 &&
> > +                disk->info.type !=
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO &&
> > +                disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
> > +                return 1;
> 
> return false;
> 
> > +            break;
> > +        case VIR_DOMAIN_DISK_BUS_XEN:
> > +        case VIR_DOMAIN_DISK_BUS_UML:
> > +        case VIR_DOMAIN_DISK_BUS_SD:
> > +            /* no address type currently, return false if address provided
> > */
> > +            return 1;
> 
> return false;
> 
> > +        }
> > +    }
> > +    return 0;
> 
> return true;
> 
> > +}
> > +
> >  virDomainDeviceInfoPtr
> >  virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
> >  {
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index f043554..337fe51 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -2543,6 +2543,7 @@ virDomainDeviceDefPtr
> > virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
> >                                               virDomainXMLOptionPtr
> >                                               xmlopt);
> >  int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
> >                                    int type);
> > +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk);
> 
> s/int/bool, probably unnecessary though
> 
> >  virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr
> >  device);
> >  int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
> >                              virDomainDeviceInfoPtr src);
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 5b3da83..6cd5b9e 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -229,6 +229,7 @@ virDomainDefPostParse;
> >  virDomainDefSetMemoryInitial;
> >  virDomainDeleteConfig;
> >  virDomainDeviceAddressIsValid;
> > +virDomainDeviceAddressTypeIsValid;
> 
> probably unnecessary
> 
> >  virDomainDeviceAddressTypeToString;
> >  virDomainDeviceDefCheckUnsupportedMemoryDevice;
> >  virDomainDeviceDefCopy;
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 25f57f2..56ba08d 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3606,6 +3606,13 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
> >              goto error;
> >          }
> >      }
> > +
> > +    if (virDomainDeviceAddressTypeIsValid(disk)) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("disk cannot have an address of type '%s'"),
> > +
> > virDomainDeviceAddressTypeToString(disk->info.type));
> 
> how about disk target '%s' using bus '%s' cannot have an address of type
> '%s', where target would be disk->dst and the bus would be decoded via
> virDomainDiskBusTypeToString(disk->bus), results in error message such as:
> 
> error: unsupported configuration: disk target 'vde' using bus 'virtio'
> cannot have an address of type 'drive'

Thanks, the message is clear now.

> 
> > +        goto error;
> > +    }
> 
> But I'm not convinced the check should be put it here.
> 
> >      return 0;
> >   error:
> >      return -1;
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index fcf86b6..26c1502 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -8208,6 +8208,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> >      switch ((virDomainDeviceType) dev->type) {
> >      case VIR_DOMAIN_DEVICE_DISK:
> >          disk = dev->data.disk;
> > +        if (qemuCheckDiskConfig(disk) < 0)
> > +            return -1;
> 
> Why are we checking at Detach?  Doesn't that already fail?  Or is this
> just to get the "new" error message?

If we just want to detach a disk without attaching it before, it's necessary to
check the configuration here.

> 
> >          if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) {
> >              virReportError(VIR_ERR_INVALID_ARG,
> >                             _("no target device %s"), disk->dst);
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index e84a78d..6156243 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -336,6 +336,28 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
> >          if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info,
> >          priv->qemuCaps,
> >                                              disk->dst))
> >              goto cleanup;
> > +
> > +        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;
> > +            }
> > +        } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390))
> > {
> > +            if (!virDomainDeviceAddressIsValid(&disk->info,
> > +
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390))
> > {
> > +                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > +                               _("device cannot be attached without a
> > valid S390 address"));
> > +                goto cleanup;
> > +            }
> 
> The whole purpose of qemuCheckCCWS390AddressSupport is to avoid adding
> more places where these type lines need to be added.

Yes, thanks.

> 
> John
> 
> > +        } 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"));
> > +                goto cleanup;
> > +        }
> >      }
> >  
> >      for (i = 0; i < vm->def->ndisks; i++) {
> > 
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]