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.crobi...@redhat.com>
In-Reply-To: <b477d397deac304be61d60b4fc4592b4faa957ab.1548192116.git.crobi...@redhat.com>
References: <b477d397deac304be61d60b4fc4592b4faa957ab.1548192116.git.crobi...@redhat.com>
From: Cole Robinson <[email protected]>
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 <[email protected]>
---
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.crobi...@redhat.com>
From: Cole Robinson <[email protected]>
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 <[email protected]>
---
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
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
