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

Re: [libvirt] [PATCH 16/18] qemu: Support scsi controller model=virtio-{non-}transitional



On 01/22/2019 12:39 PM, Cole Robinson wrote:
> On 01/21/2019 11:49 AM, Andrea Bolognani wrote:
>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>> Add <controller type='scsi' model handling for virtio transitional
>>> devices. Ex:
>>>
>>>   <controller type='scsi' model='virtio-transitional'/>
>>>
>>> * "virtio-transitional" maps to qemu "virtio-scsi-pci-transitional"
>>> * "virtio-non-transitional" maps to qemu "virtio-scsi-non-transitional"
>>>
>>> The naming here doesn't match the pre-existing model=virtio-scsi.
>>> The prescence of '-scsi' there seems kind of redundant as we have
>>> type='scsi' already, so I decided to follow the pattern of other
>>> patches and use virtio-transitional etc.
>>
>> Completely agree with the rationale here; however, in order to
>> really match the way other devices are handled, I think we should
>> make it so you can use
>>
>>   <controller type='scsi' model='virtio'/>
>>
>> as well, which of course would behave the same as the currently
>> available version. What do you think?
>>
> 
> Yes I agree it would be nice to add that option. I suggest we make it a
> separate issue from this series though incase it's a contentious idea,
> v2 is already shaping up to be ~30 patches...
> 
>>
>>> +        case VIR_DOMAIN_DEVICE_CONTROLLER:
>>> +            if (strstr(baseName, "scsi")) {
>>> +                has_tmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL;
>>> +                has_ntmodel = model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL;
>>> +                tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_TRANSITIONAL;
>>> +                ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_SCSI_NON_TRANSITIONAL;
>>> +                break;
>>> +            }
>>> +            return 0;
>>
>> Using strstr() here is kinda crude, especially since the caller has
>> enough information to pass the appropriate virDomainControllerType
>> value, both in this case and later on for serial controllers.
>>
>> I'd say just add yet another argument to the function. Yeah, it
>> starts to get quite unsightly, but I'd really rather not resort to
>> string comparison when a nice, type safe enum will do.
>>
> 
> Yeah it's hacky. Adding another arg here is going to add extra pain if
> when this is merged into qemuBuildVirtioStr, most callers are going have
> NULL arguments, and an increased chance of new future callers passing in
> incorrect values and accidentally triggering a wrong code path. I feel
> like this is another argument for the separated BuildTransitional or
> whatever, but I'm not sold either way so I'll stick with your suggestion
> 

What do you think of this approach? See the attached two patches. It
adds a domain_conf.c function virDomainDeviceSetData which makes it
easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now
pass in a virDomainDeviceType and the specific DefPtr for their device
(virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr
then turns that into a virDomainDeviceDef and acts on that locally.

Saves having to extend the argument list several times (like this
example above, and the net->model string example). Seperately I think
virDomainDeviceSetData can be used to clean up some device interactions
elsewhere too

Thanks,
Cole
>From 26a76becc0297a4fbae572ac328e9068a0141174 Mon Sep 17 00:00:00 2001
Message-Id: <26a76becc0297a4fbae572ac328e9068a0141174 1548192116 git crobinso redhat com>
In-Reply-To: <b477d397deac304be61d60b4fc4592b4faa957ab 1548192116 git crobinso redhat com>
References: <b477d397deac304be61d60b4fc4592b4faa957ab 1548192116 git crobinso redhat com>
From: Cole Robinson <crobinso redhat com>
Date: Tue, 22 Jan 2019 16:15:03 -0500
Subject: [PATCH 2/2] qemu: command: Make BuildVirtioDevStr more generic

Switch qemuBuildVirtioDevStr to use virDomainDeviceSetData: callers
pass in the virDomainDeviceType and the void * DefPtr. This will
save us from having to repeatedly extend the function argument
list in subsequent patches.

Signed-off-by: Cole Robinson <crobinso redhat com>
---
 src/qemu/qemu_command.c | 141 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 124 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb4eb97da4..ef0b6399ad 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -396,12 +396,93 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
 }
 
 
+/**
+ * qemuBuildVirtioDevStr
+ * @buf: virBufferPtr to append the built string
+ * @baseName: qemu virtio device basename string. Ex: virtio-rng for <rng>
+ * @qemuCaps: virQEMUCapPtr
+ * @devtype: virDomainDeviceType of the device. Ex: VIR_DOMAIN_DEVICE_TYPE_RNG
+ * @devdata: *DefPtr of the device definition
+ *
+ * Build the qemu virtio -device name from the passed parameters. Currently
+ * this is mostly about attaching the correct string prefix to @baseName for
+ * the passed @type. So for @baseName "virtio-rng" and devdata->info.type
+ * VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, generate "virtio-rng-pci"
+ *
+ * Returns: -1 on failure, 0 on success
+ */
 static int
 qemuBuildVirtioDevStr(virBufferPtr buf,
                       const char *baseName,
-                      virDomainDeviceAddressType type)
+                      virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
+                      virDomainDeviceType devtype,
+                      void *devdata)
 {
     const char *implName = NULL;
+    virDomainDeviceAddressType type = 0;
+    virDomainDeviceDef device = { .type = devtype };
+
+    virDomainDeviceSetData(&device, devdata);
+
+    switch ((virDomainDeviceType) device.type) {
+    case VIR_DOMAIN_DEVICE_DISK:
+        type = device.data.disk->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_NET:
+        type = device.data.net->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_FS:
+        type = device.data.fs->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_INPUT:
+        type = device.data.input->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_VIDEO:
+        type = device.data.video->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_HOSTDEV:
+        type = device.data.hostdev->info->type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_CONTROLLER:
+        type = device.data.controller->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_MEMBALLOON:
+        type = device.data.memballoon->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_RNG:
+        type = device.data.rng->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_VSOCK:
+        type = device.data.vsock->info.type;
+        break;
+
+    case VIR_DOMAIN_DEVICE_CHR:
+    case VIR_DOMAIN_DEVICE_LEASE:
+    case VIR_DOMAIN_DEVICE_SOUND:
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+    case VIR_DOMAIN_DEVICE_GRAPHICS:
+    case VIR_DOMAIN_DEVICE_HUB:
+    case VIR_DOMAIN_DEVICE_REDIRDEV:
+    case VIR_DOMAIN_DEVICE_SMARTCARD:
+    case VIR_DOMAIN_DEVICE_NVRAM:
+    case VIR_DOMAIN_DEVICE_SHMEM:
+    case VIR_DOMAIN_DEVICE_TPM:
+    case VIR_DOMAIN_DEVICE_PANIC:
+    case VIR_DOMAIN_DEVICE_MEMORY:
+    case VIR_DOMAIN_DEVICE_IOMMU:
+    case VIR_DOMAIN_DEVICE_NONE:
+    case VIR_DOMAIN_DEVICE_LAST:
+        return 0;
+    }
 
     switch (type) {
     case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
@@ -443,7 +524,6 @@ qemuBuildVirtioDevStr(virBufferPtr buf,
     return 0;
 }
 
-
 static int
 qemuBuildVirtioOptionsStr(virBufferPtr buf,
                           virDomainVirtioOptionsPtr virtio,
@@ -2050,8 +2130,10 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
         break;
 
     case VIR_DOMAIN_DISK_BUS_VIRTIO:
-        if (qemuBuildVirtioDevStr(&opt, "virtio-blk", disk->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&opt, "virtio-blk", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_DISK, disk) < 0) {
             goto error;
+        }
 
         if (disk->iothread)
             virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread);
@@ -2639,8 +2721,10 @@ qemuBuildFSDevStr(const virDomainDef *def,
         goto error;
     }
 
-    if (qemuBuildVirtioDevStr(&opt, "virtio-9p", fs->info.type) < 0)
+    if (qemuBuildVirtioDevStr(&opt, "virtio-9p", qemuCaps,
+                              VIR_DOMAIN_DEVICE_FS, fs) < 0) {
         goto error;
+    }
 
     virBufferAsprintf(&opt, ",id=%s", fs->info.alias);
     virBufferAsprintf(&opt, ",fsdev=%s%s",
@@ -2845,8 +2929,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
         switch ((virDomainControllerModelSCSI) def->model) {
         case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
-            if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", def->info.type) < 0)
+            if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", qemuCaps,
+                                      VIR_DOMAIN_DEVICE_CONTROLLER, def) < 0) {
                 goto error;
+            }
 
             if (def->iothread) {
                 virBufferAsprintf(&buf, ",iothread=iothread%u",
@@ -2886,8 +2972,10 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
         break;
 
     case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-serial", def->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-serial", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_CONTROLLER, def) < 0) {
             goto error;
+        }
 
         virBufferAsprintf(&buf, ",id=%s", def->info.alias);
         if (def->opts.vioserial.ports != -1) {
@@ -3655,8 +3743,10 @@ qemuBuildNicDevStr(virDomainDefPtr def,
     char macaddr[VIR_MAC_STRING_BUFLEN];
 
     if (virDomainNetIsVirtioModel(net)) {
-        if (qemuBuildVirtioDevStr(&buf, "virtio-net", net->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-net", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_NET, net) < 0) {
             goto error;
+        }
 
         usingVirtio = true;
     } else {
@@ -4049,8 +4139,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd,
     if (!virDomainDefHasMemballoon(def))
         return 0;
 
-    if (qemuBuildVirtioDevStr(&buf, "virtio-balloon",
-                              def->memballoon->info.type) < 0) {
+    if (qemuBuildVirtioDevStr(&buf, "virtio-balloon", qemuCaps,
+                              VIR_DOMAIN_DEVICE_MEMBALLOON,
+                              def->memballoon) < 0) {
         goto error;
     }
 
@@ -4148,20 +4239,28 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def,
 
     switch ((virDomainInputType)dev->type) {
     case VIR_DOMAIN_INPUT_TYPE_MOUSE:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-mouse", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-mouse", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_TABLET:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-tablet", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-tablet", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_KBD:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-keyboard", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-keyboard", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
-        if (qemuBuildVirtioDevStr(&buf, "virtio-input-host", dev->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-input-host", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_INPUT, dev) < 0) {
             goto error;
+        }
         break;
     case VIR_DOMAIN_INPUT_TYPE_LAST:
     default:
@@ -4479,8 +4578,10 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
     }
 
     if (STREQ(model, "virtio-gpu")) {
-        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", video->info.type) < 0)
+        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", qemuCaps,
+                                  VIR_DOMAIN_DEVICE_VIDEO, video) < 0) {
             goto error;
+        }
     } else {
         virBufferAsprintf(&buf, "%s", model);
     }
@@ -4925,8 +5026,10 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def,
         goto cleanup;
     }
 
-    if (qemuBuildVirtioDevStr(&buf, "vhost-scsi", dev->info->type) < 0)
+    if (qemuBuildVirtioDevStr(&buf, "vhost-scsi", qemuCaps,
+                              VIR_DOMAIN_DEVICE_HOSTDEV, dev) < 0) {
         goto cleanup;
+    }
 
     virBufferAsprintf(&buf, ",wwpn=%s,vhostfd=%s,id=%s",
                       hostsrc->wwpn,
@@ -5873,8 +5976,10 @@ qemuBuildRNGDevStr(const virDomainDef *def,
                                               dev->source.file))
         goto error;
 
-    if (qemuBuildVirtioDevStr(&buf, "virtio-rng", dev->info.type) < 0)
+    if (qemuBuildVirtioDevStr(&buf, "virtio-rng", qemuCaps,
+                              VIR_DOMAIN_DEVICE_RNG, dev) < 0) {
         goto error;
+    }
 
     virBufferAsprintf(&buf, ",rng=obj%s,id=%s",
                       dev->info.alias, dev->info.alias);
@@ -10337,8 +10442,10 @@ qemuBuildVsockDevStr(virDomainDefPtr def,
     char *ret = NULL;
 
 
-    if (qemuBuildVirtioDevStr(&buf, "vhost-vsock", vsock->info.type) < 0)
+    if (qemuBuildVirtioDevStr(&buf, "vhost-vsock", qemuCaps,
+                              VIR_DOMAIN_DEVICE_VSOCK, vsock) < 0) {
         goto cleanup;
+    }
 
     virBufferAsprintf(&buf, ",id=%s", vsock->info.alias);
     virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid);
-- 
2.20.1

>From b477d397deac304be61d60b4fc4592b4faa957ab Mon Sep 17 00:00:00 2001
Message-Id: <b477d397deac304be61d60b4fc4592b4faa957ab 1548192116 git crobinso redhat com>
From: Cole Robinson <crobinso redhat com>
Date: Tue, 22 Jan 2019 15:19:29 -0500
Subject: [PATCH 1/2] conf: Add virDomainDeviceSetData

This is essentially a wrapper for easily setting the variable
name in virDomainDeviceDef that matches its associated
VIR_DOMAIN_DEVICE_TYPE.

Signed-off-by: Cole Robinson <crobinso redhat com>
---
 src/conf/domain_conf.c   | 93 ++++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h   |  3 ++
 src/libvirt_private.syms |  1 +
 3 files changed, 97 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index dc17a2fd59..313e1c1748 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3701,6 +3701,99 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def,
 }
 
 
+/**
+ * virDomainDeviceSetData
+ * @device: virDomainDeviceDefPtr with ->type filled in
+ * @data: *DefPtr data for a device. Ex: virDomainDiskDefPtr
+ *
+ * Set the data.X variable for the device->type value. Basically
+ * a mapping of virDomainDeviceType to the associated name in
+ * the virDomainDeviceDef union
+ */
+void
+virDomainDeviceSetData(virDomainDeviceDefPtr device,
+                       void *devicedata)
+{
+    switch ((virDomainDeviceType) device->type) {
+    case VIR_DOMAIN_DEVICE_DISK:
+        device->data.disk = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_NET:
+        device->data.net = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_SOUND:
+        device->data.sound = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_HOSTDEV:
+        device->data.hostdev = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_VIDEO:
+        device->data.video = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_CONTROLLER:
+        device->data.controller = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_GRAPHICS:
+        device->data.graphics = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_SMARTCARD:
+        device->data.smartcard = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_CHR:
+        device->data.chr = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_INPUT:
+        device->data.input = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_FS:
+        device->data.fs = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+        device->data.watchdog = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_MEMBALLOON:
+        device->data.memballoon = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_RNG:
+        device->data.rng = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_NVRAM:
+        device->data.nvram = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_HUB:
+        device->data.hub = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_SHMEM:
+        device->data.shmem = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_TPM:
+        device->data.tpm = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_PANIC:
+        device->data.panic = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_MEMORY:
+        device->data.memory = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_REDIRDEV:
+        device->data.redirdev = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_VSOCK:
+        device->data.vsock = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_IOMMU:
+        device->data.iommu = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_LEASE:
+        device->data.lease = devicedata;
+        break;
+    case VIR_DOMAIN_DEVICE_NONE:
+    case VIR_DOMAIN_DEVICE_LAST:
+        break;
+    }
+}
+
+
 enum {
     DOMAIN_DEVICE_ITERATE_ALL_CONSOLES = 1 << 0,
     DOMAIN_DEVICE_ITERATE_GRAPHICS = 1 << 1
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index dcd84e2d94..b5894b96b6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2934,6 +2934,9 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
 virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device);
 void virDomainTPMDefFree(virDomainTPMDefPtr def);
 
+void virDomainDeviceSetData(virDomainDeviceDefPtr device,
+                            void *devicedata);
+
 typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
                                            virDomainDeviceDefPtr dev,
                                            virDomainDeviceInfoPtr info,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6f4809a68a..89b8ca3b4f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -307,6 +307,7 @@ virDomainDeviceDefParse;
 virDomainDeviceFindSCSIController;
 virDomainDeviceGetInfo;
 virDomainDeviceInfoIterate;
+virDomainDeviceSetData;
 virDomainDeviceTypeToString;
 virDomainDeviceValidateAliasForHotplug;
 virDomainDiskBusTypeToString;
-- 
2.20.1


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