[libvirt] [PATCH v2 1/5] Introduce a "scsi_host" hostdev type
Eric Farman
farman at linux.vnet.ibm.com
Tue Sep 13 18:02:44 UTC 2016
On 09/12/2016 05:34 PM, John Ferlan wrote:
> On 09/06/2016 08:58 AM, Eric Farman wrote:
>> We already have a "scsi" hostdev type, which refers to a single LUN
>> that is passed through to a guest. But what of things where multiple
>> LUNs are passed through via a single SCSI HBA, such as with the
>> vhost-scsi target? Create a new hostdev type that will carry
>> this, and its associated documentation and XML schema information.
>>
> I assume from the review of v1 this is both HBA and vHBA? That is a
> vHBA would be useful for the NPIV support to pass through the entire
> vHBA rather than individual LUN's (something recently asked for at KVM
> Forum).
I wasn't at KVM Forum, so I'm in the dark here. :(
> The vHBA is created when one has a vport capable scsi_host (see
> http://wiki.libvirt.org/page/NPIV_in_libvirt). The "result" is a
> 'scsi_hostM' using a vport capable parent scsi_hostN. Now if a vHBA
> cannot (or should not) be passed through, then we'll need to be sure to
> test and avoid it. Unfortunately we had a lab issue and the system I
> normally use for my vHBA/NPIV work was affected - trying to get it
> resurected remotely...
My experience with NPIV has been on s390, so I'm gonna plead some
ignorance on NPIV for other platforms. Considering the lack of controls
on the qemu side, I would imagine that it should work.
>
>> Signed-off-by: Eric Farman <farman at linux.vnet.ibm.com>
>> ---
>> docs/formatdomain.html.in | 24 ++++++++++++++++
>> docs/schemas/domaincommon.rng | 23 +++++++++++++++
>> src/conf/domain_audit.c | 2 ++
>> src/conf/domain_conf.c | 56 +++++++++++++++++++++++++++++++++++--
>> src/conf/domain_conf.h | 17 +++++++++++
>> src/qemu/qemu_domain_address.c | 10 +++++++
>> src/qemu/qemu_hotplug.c | 1 +
>> src/security/security_dac.c | 2 ++
>> tests/domaincapsschemadata/full.xml | 1 +
>> 9 files changed, 134 insertions(+), 2 deletions(-)
>>
> I took a peek at the v1, but suffice to say didn't follow it that
> closely since there's a number of changes in v2 related to patch
> ordering. You could get 3 different opinions on that matter too!
Hopefully a consensus before too long. :)
> Typically I'll try to add the qemu code first (capabilities first, then
> command in one patch and hotplug in the next one). Once that's in place,
> then I add the domain bits, docs, etc.
>
> Since you're extending the domain XML, you will need to add an xml2xml
> test in this patch. Use the XML from your patch 5 as a basis/guide.
> You'll need a "tests/qemuxml2argvdata/" file and a
> "tests/qemuxml2xmloutdata/" file as well as a changed to
> qemuxml2xmltest.c. If the data file is the same as the outdata file,
> then you can create a soft link to the data file in the outdata file
> directory.
>
>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index feaeaa2..b79da95 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -3655,6 +3655,17 @@
>> </devices>
>> ...</pre>
>>
>> + <p>or:</p>
>> +
>> +<pre>
>> + ...
>> + <devices>
>> + <hostdev mode='subsystem' type='scsi_host'>
>> + <source protocol='vhost' wwpn='naa.50014057667280d8'/>
>> + </hostdev>
>> + </devices>
>> + ...</pre>
>> +
> A wwpn doesn't generally have the "naa." prefix on it. So, why not just
> add that to the command line instead? That is a wwpn is a 16 hex digit
> value. I'm assuming that in order to support this feature a "naa." is
> prefixed and sent to qemu. Does that necessarily mean some other
> hypervisor would choose to place the "naa." prefix or would then code
> need to be added to strip it off for those other hypervisors? Looking
> at the qemu code - it seems fairly specific.
Fairly certain that it's more a vhost thing of which the hypervisor
utilizes. Per a recommendation made on this list years ago, I used
targetcli to do the configuration rather than direct manipulation of
sysfs, and according to its man page:
All WWNs are prefaced by their type, such as "iqn", "naa", or "ib",
and may be given with or without colons.
As to whether other hypervisors follow the same behavior, I do not
know. I was just trying to conform to what it would allow in the case
of vhost-scsi. Now, I suppose I'm confusing matters by saying "wwpn"
here since as you say that's 16 hex digits, and should probably say just
"wwn" ?
>
>
>> <dl>
>> <dt><code>hostdev</code></dt>
>> <dd>The <code>hostdev</code> element is the main container for describing
>> @@ -3693,6 +3704,12 @@
>> If a disk lun in the domain already has the rawio capability,
>> then this setting not required.
>> </dd>
>> + <dt><code>scsi_host</code></dt>
>> + <dd><span class="since">since 2.2.0</span>For SCSI devices, user
>> + is responsible to make sure the device is not used by host. This
>> + <code>type</code> passes all LUNs presented by a single HBA to
>> + the guest.
>> + </dd>
> It'll be at least 2.3.0
Ugh, yeah. When I rebased after 2.2's release I forgot about the doc
hits. My apologies.
>
> I see from the qemu code there's support for attributes 'num_queues',
> 'max_sectors', and 'cmd_per_lun' - since this is passing the scsi_host
> through and not a 'scsi' LUN with a 'scsi' controller, should those
> attributes be added as well? Not "required" for this version, but I
> would hope we could add caps and attributes for that as well because I
> can almost guarantee someone will ask.
>
>> </dl>
>> <p>
>> Note: The <code>managed</code> attribute is only used with PCI devices
>> @@ -3756,6 +3773,13 @@
>> credentials to the iSCSI server.
>> </p>
>> </dd>
>> + <dt><code>scsi_host</code></dt>
>> + <dd><span class="since">Since 2.2.0</span>, multiple LUNs behind a
> 2.3
>
>> + single SCSI HBI are described by a <code>protocol</code>
> s/HBI/HBA
>
>> + attribute set to "vhost" and a <code>wwpn</code> attribute that
>> + is the vhost_scsi wwpn (16 hexadecimal digits with a prefix of
>> + "naa.") established in the host configfs.
>> + </dd>
> I don't think we mention the "naa." prefix...
>
> It would be nice to tell a user how to find that wwpn value via some
> command (virsh or otherwise) in order to fill in the wwpn attribute.
> Although there's always varying opinions on this since many times it's
> distro dependent. That's why I suggest virsh - at least if it's
> supportable we can display the wwpn there using nodedev-dumpxml.
Hrm... nodedev-dumpxml is new to me, neat! I'll go down that bunny
trail, but it seems like it'd be a bit of work to add the smarts for this.
>
> What/where is the 'vhost_scsi_wwpn'? IOW: It's not something defined
> yet - I'm assuming there's some output somewhere where that's pulled from.
As near as I can tell, it's not anything that can be autogenerated off
the bat. Somebody somewhere needs to first establish the sysfs entries
for the vhost target, which can then be stored and loaded on a next
(host) boot. The end result looks something like this:
[root at xxxxxxxx ~]# ls -l
/sys/kernel/config/target/vhost/naa.50014056e4794195/tpgt_1/lun/lun_0/
total 0
lrwxrwxrwx 1 root root 0 Sep 13 15:44 752e150d11 ->
../../../../../../target/core/iblock_0/disk0
-rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_gp
-rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_offline
-rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_status
-rw-r--r-- 1 root root 4096 Sep 13 15:45 alua_tg_pt_write_md
drwxr-xr-x 5 root root 0 Sep 13 15:44 statistics
[root at xxxxxxxx ~]# cat /sys/kernel/config/target/core/iblock_0/disk0/info
Status: ACTIVATED Max Queue Depth: 0 SectorSize: 512 HwMaxSectors: 4592
iBlock device: dm-0 UDEV PATH: /dev/mapper/mpathb readonly: 0
Major: 252 Minor: 0 CLAIMED: IBLOCK
(Aside: note the folder name in /sys/kernel/config that contains the
wwn/wwpn name with the "naa." prefix.)
>
> One final question - what is this exposed as on the guest? "Some"
> scsi_hostM? For a SCSI LUN it's some 'sgN' value. I see the qemu side
> seems to infer some amount of scsi_generic code (read_naa_id), so it's
> mostly curiosity/learning for me.
Anything that's put behind the target gets seen to the guest. So that
can be one or more sgN LUNs and their sdX equivalents, and as near as I
can tell a scsi_hostM as you describe. A quick test with two LUNs
behind one vhost-scsi target gives me the following in the guest:
# ls -l /sys/bus/scsi/devices
total 0
lrwxrwxrwx 1 root root 0 Sep 13 16:10 0:0:1:0 ->
../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0/target0:0:1/0:0:1:0
lrwxrwxrwx 1 root root 0 Sep 13 16:10 0:0:1:1 ->
../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0/target0:0:1/0:0:1:1
lrwxrwxrwx 1 root root 0 Sep 13 16:10 host0 ->
../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0
lrwxrwxrwx 1 root root 0 Sep 13 16:11 target0:0:1 ->
../../../devices/css0/0.0.0002/0.0.beef/virtio2/host0/target0:0:1
# lsscsi
[0:0:1:0] disk LIO-ORG disk0 4.0 /dev/sda
[0:0:1:1] disk LIO-ORG disk1 4.0 /dev/sdb
FYI, hotplugging/unplugging devices to a target works just fine, except
there's no notification that a guest can pick up on to know that a
device had been added. Or worse, taken away.
>
>> </dl>
>> </dd>
>> <dt><code>vendor</code>, <code>product</code></dt>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index af32df8..7fd6bd2 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -3938,6 +3938,7 @@
>> <ref name="hostdevsubsyspci"/>
>> <ref name="hostdevsubsysusb"/>
>> <ref name="hostdevsubsysscsi"/>
>> + <ref name="hostdevsubsyshost"/>
>> </choice>
>> </define>
>>
>> @@ -4066,6 +4067,28 @@
>> </element>
>> </define>
>>
>> + <define name="hostdevsubsyshost">
>> + <attribute name="type">
>> + <value>scsi_host</value>
>> + </attribute>
>> + <element name="source">
>> + <choice>
>> + <group>
>> + <attribute name="protocol">
>> + <choice>
>> + <value>vhost</value> <!-- vhost, required -->
>> + </choice>
>> + </attribute>
>> + <attribute name="wwpn">
>> + <data type="string">
>> + <param name="pattern">(naa\.)[0-9a-fA-F]{16}</param>
>> + </data>
> If you go with the we'll add "naa." later, then you could just:
>
> <ref name='wwn'/>
This rings a bell with me, as to why I had the label be "wwpn" instead
of "wwn" above. My attempt at trying to distinguish libvirt internals
with something user-facing, maybe?
>
> instead of <data ...> ... </data>
>
>> + </attribute>
>> + </group>
>> + </choice>
>> + </element>
>> + </define>
>> +
>> <define name="hostdevcapsstorage">
>> <attribute name="type">
>> <value>storage</value>
>> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
>> index 53a58ac..406dd8f 100644
>> --- a/src/conf/domain_audit.c
>> +++ b/src/conf/domain_audit.c
>> @@ -443,6 +443,8 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
>> }
>> break;
>> }
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> Can't we produce something here? Not sure seeing "?" in the output of an
> audit message would be useful. Perhaps the path to the device similar to
> how we end up in virDomainAuditGenericDev...
Fair enough. Will go down that trail.
>
>> + break;
>> default:
>> VIR_WARN("Unexpected hostdev type while encoding audit message: %d",
>> hostdev->source.subsys.type);
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index c8c4f61..45b643b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -646,7 +646,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,
>> VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST,
>> "usb",
>> "pci",
>> - "scsi")
>> + "scsi",
>> + "scsi_host")
>>
>> VIR_ENUM_IMPL(virDomainHostdevSubsysPCIBackend,
>> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST,
>> @@ -660,6 +661,10 @@ VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol,
>> "adapter",
>> "iscsi")
>>
>> +VIR_ENUM_IMPL(virDomainHostdevSubsysHostProtocol,
>> + VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_LAST,
>> + "vhost")
>> +
> Notice how the virDomainHostdevSubsysSCSIProtocol adds 'adapter' first -
> this should do something similar (although the preference is to use
> "none" - I cannot recall why that was different).
Because "none" == "adapter" in the case of SCSI?
>
> Remember that all structs are "calloc"'d and thus making sure this
> setting to something other than 0 will ensure we haven't missed something.
>
>
>> VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST,
>> "storage",
>> "misc",
>> @@ -2270,6 +2275,9 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
>> } else {
>> VIR_FREE(scsisrc->u.host.adapter);
>> }
>> + } else if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) {
>> + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
>> + VIR_FREE(hostsrc->wwpn);
>> }
>> break;
>> }
>> @@ -5977,6 +5985,31 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode,
>> return ret;
>> }
>>
>> +static int
>> +virDomainHostdevSubsysHostDefParseXML(xmlNodePtr sourcenode,
>> + virDomainHostdevDefPtr def)
>> +{
>> + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
>> +
> This code would do the "protocol" parsing. I believe the way it's
> written now protocol goes unchecked. You need to add code to use the
> virDomainHostdevSubsysHostProtocolTypeFromString in order to validate
> what was supplied in the XML is correct.
>
> See virDomainHostdevSubsysSCSIDefParseXML - although make your
> ->protocol comparison <= 0 since we don't want a "0" value.
>
> (Yes, there's a bug in the SCSIDefParse I think - although it's caught
> later on).
>
>> + if (!(hostsrc->wwpn = virXMLPropString(sourcenode, "wwpn"))) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("missing vhost-scsi hostdev source path name"));
>> + goto cleanup;
>> + }
> If we take the don't require a "naa." prefix, then virValidateWWN should
> be used to validate things. Even if naa. was kept, the same validation
> should be done for consistency.
Ah, okay. virValidateWWN can be used regardless I suppose.
>
>> +
>> + if (!STRPREFIX(hostsrc->wwpn, "naa.") ||
>> + strlen(hostsrc->wwpn) != strlen("naa.") + 16) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed 'wwpn' value"));
>> + goto cleanup;
>> + }
>> +
>> + return 0;
>> +
>> + cleanup:
>> + VIR_FREE(hostsrc->wwpn);
>> + return -1;
>> +}
>> +
>>
>> static int
>> virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>> @@ -6101,6 +6134,11 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
>> goto error;
>> break;
>>
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> + if (virDomainHostdevSubsysHostDefParseXML(sourcenode, def) < 0)
>> + goto error;
>> + break;
>> +
>> default:
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("address type='%s' not supported in hostdev interfaces"),
>> @@ -20521,9 +20559,11 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> unsigned int flags,
>> bool includeTypeInAddr)
>> {
>> + bool closedSource = false;
>> virDomainHostdevSubsysUSBPtr usbsrc = &def->source.subsys.u.usb;
>> virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci;
>> virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi;
>> + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
>> virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
>> virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
>>
>> @@ -20564,6 +20604,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> protocol, iscsisrc->path);
>> }
>>
>> + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) {
> FWIW: If some day in the future type='scsi_host' added some other
> "protocol" (something other than vhost), then at that time this code
> would be extended to handle that.
>
>> + const char *protocol =
>> + virDomainHostdevSubsysHostProtocolTypeToString(hostsrc->protocol);
>> + closedSource = true;
>> +
>> + virBufferAsprintf(buf, " protocol='%s' wwpn='%s'/",
>> + protocol, hostsrc->wwpn);
>> + }
>> +
>> virBufferAddLit(buf, ">\n");
>>
>> virBufferAdjustIndent(buf, 2);
>> @@ -20617,6 +20666,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> scsihostsrc->unit);
>> }
>> break;
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> + break;
>> default:
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("unexpected hostdev type %d"),
>> @@ -20632,7 +20683,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>> }
>>
>> virBufferAdjustIndent(buf, -2);
>> - virBufferAddLit(buf, "</source>\n");
>> + if (!closedSource)
>> + virBufferAddLit(buf, "</source>\n");
>>
>> return 0;
>> }
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 0fe4154..781ea7b 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -294,6 +294,7 @@ typedef enum {
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB,
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI,
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
>> + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST,
>>
>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST
>> } virDomainHostdevSubsysType;
>> @@ -368,6 +369,21 @@ struct _virDomainHostdevSubsysSCSI {
>> } u;
>> };
>>
>> +typedef enum {
> VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_NONE,
>
> Add the NONE here.
>
>> + VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_VHOST,
>> +
>> + VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_LAST,
>> +} virDomainHostdevHostProtocolType;
>> +
>> +VIR_ENUM_DECL(virDomainHostdevSubsysHostProtocol)
>> +
>> +typedef struct _virDomainHostdevSubsysHost virDomainHostdevSubsysHost;
>> +typedef virDomainHostdevSubsysHost *virDomainHostdevSubsysHostPtr;
>> +struct _virDomainHostdevSubsysHost {
>> + int protocol;
>> + char *wwpn;
>> +};
>> +
>> typedef struct _virDomainHostdevSubsys virDomainHostdevSubsys;
>> typedef virDomainHostdevSubsys *virDomainHostdevSubsysPtr;
>> struct _virDomainHostdevSubsys {
>> @@ -376,6 +392,7 @@ struct _virDomainHostdevSubsys {
>> virDomainHostdevSubsysUSB usb;
>> virDomainHostdevSubsysPCI pci;
>> virDomainHostdevSubsysSCSI scsi;
>> + virDomainHostdevSubsysHost host;
>> } u;
>> };
>>
>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>> index bb16738..8419e6a 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -301,6 +301,16 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
>> def->controllers[i]->info.type = type;
>> }
>>
>> + for (i = 0; i < def->nhostdevs; i++) {
>> + if (def->hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>> + def->hostdevs[i]->source.subsys.type ==
>> + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST &&
>> + def->hostdevs[i]->source.subsys.u.host.protocol ==
>> + VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_VHOST &&
>> + def->hostdevs[i]->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>> + def->hostdevs[i]->info->type = type;
>> + }
>> +
>> if (def->memballoon &&
>> def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
>> def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index d13474a..e9140a9 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3238,6 +3238,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>> qemuDomainRemoveUSBHostDevice(driver, vm, hostdev);
>> break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> hmmm... If you're going to change this here, then
> qemuDomainAttachHostDevice would be changed at the same time. Hence why
> this ordering stuff to patches is important.
>
> I cannot recall if removing would cause a build error in this case.
It does, because of the switch statement not having a "default" case.
>
>> qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev);
>> break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 442ce70..de2b921 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -676,6 +676,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr,
>> break;
>> }
>>
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>> ret = 0;
>> break;
>> @@ -805,6 +806,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr,
>> break;
>> }
>>
>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
>> ret = 0;
>> break;
> Why is only security_dac.c changed? Usually _selinux and _apparmor.c
> are also changed, which I do see happening in your v1.
Oh darn, what happened. :( I'll go revisit my merging/rebasing.
> Although I
> wouldn't classify a "vhost" and an "iSCSI" host as the same thing. The
> latter is a networked resource; whereas, IIUC this new device is
> essentially a file descriptor that's being passed along to qemu. So
> there must be other examples of that in the security_*.c modules.
>
> This particular area I usually don't get right either, but what I'll do
> is follow the history of the changes and see if something is "similar"
> to what I'm creating and then just copy that model.
>
> Makes me wonder how this plays in a non-privileged environment (e.g.
> session mode). Is it even allowed to open/read vhost-scsi.
Excellent question. I'll give it a try.
>
>> diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml
>> index 2f529ff..41fb1fa 100644
>> --- a/tests/domaincapsschemadata/full.xml
>> +++ b/tests/domaincapsschemadata/full.xml
>> @@ -75,6 +75,7 @@
>> <value>usb</value>
>> <value>pci</value>
>> <value>scsi</value>
>> + <value>scsi_host</value>
>> </enum>
>> <enum name='capsType'>
>> <value>storage</value>
>>
> FWIW: I usually use cscope to find all VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_
> occurrances and be sure the _HOST option is covered. I see that nothing
> in cgroup is modified. I also wonder if we need to track these in
> virhostdev.c.
>
> John
>
Thanks for the review. (Silent ACK on a lot of the above comments.)
I'll try to get these all in place for a v3 with a little runway before
the next freeze.
- Eric
More information about the libvir-list
mailing list