Re: [libvirt] [PATCH] qemu: Forbid "sgio" support for SCSI generic host device

On Wed, Mar 12, 2014 at 23:13:24 +0800, Osier Yang wrote: > On 12/03/14 21:54, Jiri Denemark wrote: >> On Fri, Mar 07, 2014 at 22:23:26 +0800, Osier Yang wrote: >>> The kernel didn't support the unprivileged SGIO for SCSI generic >>> device finally, and since it's unknow whether the way to support >>> unprivileged SGIO for SCSI generic device will be similar as for >>> SCSI block device or not, even it's simliar (I.e. via sysfs, for >>> SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio, >>> for example), the file name might be different, So it's better not >>> guess what it should be like currently. >>> >>> This patch removes the related code (mainly about the "shareable" >>> checking on the "sgio" setting, it's not supported at all, why >>> we leave checking code there? :-), and error out if "sgio" is >>> specified in the domain config. >>> --- >>> src/qemu/qemu_conf.c | 87 ++++++++++++---------------------------------------- >>> 1 file changed, 20 insertions(+), 67 deletions(-) >>> >>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >>> index 2c397b0..ad6348d 100644 >>> --- a/src/qemu/qemu_conf.c >>> +++ b/src/qemu/qemu_conf.c >> ... >>> @@ -1135,22 +1102,15 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) >>> } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { >>> hostdev = dev->data.hostdev; >>> >>> - if (!hostdev->shareable || >>> - !(hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && >>> - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) >>> - return 0; >> In the follow-up patch, you recovered just the >> >> hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI >> >> part. Shouldn't the other checks in this condition remain? >> >>> - >>> - if (!(hostdev_name = virSCSIDeviceGetDevName(NULL, >>> - hostdev->source.subsys.u.scsi.adapter, >>> - hostdev->source.subsys.u.scsi.bus, >>> - hostdev->source.subsys.u.scsi.target, >>> - hostdev->source.subsys.u.scsi.unit))) >>> - goto cleanup; >>> - >>> - if (virAsprintf(&hostdev_path, "/dev/%s", hostdev_name) < 0) >>> + if (hostdev->source.subsys.u.scsi.sgio) { >> In other words, should this be something like >> >> if (hostdev->shareable && >> hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && >> hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && >> hostdev->source.subsys.u.scsi.sgio) { >> >> > Should be: > > If (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && > hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && > hostdev->source.subsys.u.scsi.sgio) { > > Regardless of whether the SCSI generic device is shareable or not. We > won't support > the unpiv_sgio setting. I lost the SUBSYS checking in the follow up > patch indeed. If no > other issue, I can squash it in when pushing. Thanks for the reviewing. OK, makes sense. ACK with this change then.