On Mon, Apr 01, 2013 at 08:00:53PM +0800, Han Cheng wrote:

The only parameter in -drive affect scsi-generic is readonly. Introduce <readonly/> to <hostdev>. The helper function to look up disk controller model may be used by scsi hostdev. But it should be changed to use info.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- docs/formatdomain.html.in | 3 +++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 6 +++--- src/conf/domain_conf.h | 6 ++++-- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 4 ++-- 6 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cf382e8..a6bacfa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2257,6 +2257,9 @@ <code>id</code> attribute that specifies the USB vendor and product id. The ids can be given in decimal, hexadecimal (starting with 0x) or octal (starting with 0) form.</dd> + <dt><code>readonly</code></dt> + <dd>Specifies that the device is readonly. + <span class="since">Since 0.13.0</span> for SCSI devices.</dd>

0.13.0 -> 1.0.5 version 1.0.4 is just out.

<dt><code>boot</code></dt> <dd>Specifies that the device is bootable. The <code>order</code> attribute determines the order in which devices will be tried during diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8d7e6db..ccf0913 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2911,6 +2911,11 @@ <ref name="alias"/> </optional> <optional> + <element name='readonly'> + <empty/> + </element> + </optional> + <optional> <ref name="deviceBoot"/> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f3fca7f..d9d6b9f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3452,8 +3452,8 @@ error: }

int -virDomainDiskFindControllerModel(virDomainDefPtr def, - virDomainDiskDefPtr disk, +virDomainDeviceFindControllerModel(virDomainDefPtr def, + virDomainDeviceInfoPtr info, int controllerType)

Indentation.

{ int model = -1; @@ -3461,7 +3461,7 @@ virDomainDiskFindControllerModel(virDomainDefPtr def,

for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == controllerType && - def->controllers[i]->idx == disk->info.addr.drive.controller) + def->controllers[i]->idx == info->addr.drive.controller) model = def->controllers[i]->model; }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index edddf25..f8e3973 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -439,6 +439,8 @@ struct _virDomainHostdevDef { } source; virDomainHostdevOrigStates origstates; virDomainDeviceInfoPtr info; /* Guest address */ + /* readonly is only used for scsi hostdev */ + unsigned int readonly;

bool readonly;

};

/* Two types of disk backends */ @@ -1981,8 +1983,8 @@ void virDomainInputDefFree(virDomainInputDefPtr def); void virDomainDiskDefFree(virDomainDiskDefPtr def); void virDomainLeaseDefFree(virDomainLeaseDefPtr def); void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def); -int virDomainDiskFindControllerModel(virDomainDefPtr def, - virDomainDiskDefPtr disk, +int virDomainDeviceFindControllerModel(virDomainDefPtr def, + virDomainDeviceInfoPtr info, int controllerType);

Indentation.

virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def, int bus, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 96eea0a..f2eefc3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -128,6 +128,7 @@ virDomainDeviceAddressTypeToString; virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; +virDomainDeviceFindControllerModel; virDomainDeviceInfoCopy; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; @@ -145,7 +146,6 @@ virDomainDiskDeviceTypeToString; virDomainDiskErrorPolicyTypeFromString; virDomainDiskErrorPolicyTypeToString; virDomainDiskFindByBusAndDst; -virDomainDiskFindControllerModel; virDomainDiskGeometryTransTypeFromString; virDomainDiskGeometryTransTypeToString; virDomainDiskHostDefFree; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a0c278f..eac72c2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -557,7 +557,7 @@ qemuAssignDeviceDiskAliasCustom(virDomainDefPtr def, if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { controllerModel = - virDomainDiskFindControllerModel(def, disk, + virDomainInfoFindControllerModel(def, &disk->info,

s/virDomainInfoFindControllerModel/virDomainDeviceFindControllerModel/ It's a good habit to always test your patches before sending them out.

VIR_DOMAIN_CONTROLLER_TYPE_SCSI);

if ((qemuSetScsiControllerModel(def, qemuCaps, &controllerModel)) < 0) @@ -3179,7 +3179,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def, }

controllerModel = - virDomainDiskFindControllerModel(def, disk, + virDomainInfoFindControllerModel(def, &disk->info,

s/virDomainInfoFindControllerModel/virDomainDeviceFindControllerModel/

VIR_DOMAIN_CONTROLLER_TYPE_SCSI); if ((qemuSetScsiControllerModel(def, qemuCaps, &controllerModel)) < 0) goto error; -- 1.7.1

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

On 04/02/2013 11:02 AM, Han Cheng wrote:

On 04/02/2013 10:15 AM, Hu Tao wrote:

...

On Mon, Apr 01, 2013 at 08:00:53PM +0800, Han Cheng wrote:

...

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f3fca7f..d9d6b9f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3452,8 +3452,8 @@ error: }

int -virDomainDiskFindControllerModel(virDomainDefPtr def, - virDomainDiskDefPtr disk, +virDomainDeviceFindControllerModel(virDomainDefPtr def, + virDomainDeviceInfoPtr info, int controllerType)

Indentation.

I'm sorry for messing this up. Osier advised me to use virDomainDeviceFindControllerModel rather than virDomainInfoFindControllerModel. I've change the definition one but I forgot to change the referenced places. This also caused the indentation problem. I didn't pay enough time for this. I'm really sorry for this.

I'd like to say that I did some test for major object of this patch series.

What I've test: make && make check. start and shutdown guest with scsi hostdev. hotplug/hotunplug scsi hostdev with live guest. And these all passed. Sorry for confusion by the code misplacement caused by "git rebase" mistake.

On 04/05/2013 05:10 AM, Han Cheng wrote:

On 04/02/2013 10:15 AM, Hu Tao wrote:

...

On Mon, Apr 01, 2013 at 08:00:53PM +0800, Han Cheng wrote:

...

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index edddf25..f8e3973 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -439,6 +439,8 @@ struct _virDomainHostdevDef { } source; virDomainHostdevOrigStates origstates; virDomainDeviceInfoPtr info; /* Guest address */ + /* readonly is only used for scsi hostdev */ + unsigned int readonly;

bool readonly;

...

};

/* Two types of disk backends */

How about: struct _virDomainHostdevDef { ... unsigned int managed : 1; unsigned int missing : 1; unsigned int readonly : 1; /* readonly is only used for scsi hostdev */ ... };

Actually the use of bitfields for booleans has kind of fallen out of favor in libvirt, and those left in the code seem to be there just because nobody has thought it was important enough to change it. If you want consistency, I think it would be more appropriate to make a patch that changed managed and missing from bitfields into bool, then add readonly as a bool too.

On 01/04/13 20:00, Han Cheng wrote:

The only parameter in -drive affect scsi-generic is readonly. Introduce <readonly/> to <hostdev>. The helper function to look up disk controller model may be used by scsi hostdev. But it should be changed to use info.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- docs/formatdomain.html.in | 3 +++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 6 +++--- src/conf/domain_conf.h | 6 ++++-- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 4 ++-- 6 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cf382e8..a6bacfa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2257,6 +2257,9 @@ <code>id</code> attribute that specifies the USB vendor and product id. The ids can be given in decimal, hexadecimal (starting with 0x) or octal (starting with 0) form.</dd> + <dt><code>readonly</code></dt> + <dd>Specifies that the device is readonly. + <span class="since">Since 0.13.0</span> for SCSI devices.</dd>

Except the wrong version pointed out by Hu Tao. How about: <code>readonly</code> <dd>Indicates the device is readonly, only valid for SCSI device. <span class="since">Since 1.0.5</span>. </dd>

<dt><code>boot</code></dt> <dd>Specifies that the device is bootable. The <code>order</code> attribute determines the order in which devices will be tried during diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8d7e6db..ccf0913 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2911,6 +2911,11 @@ <ref name="alias"/> </optional> <optional> + <element name='readonly'> + <empty/> + </element> + </optional> + <optional>

Since the "readonly" is only valid for SCSI device. This patch should be either merged into 3/10, or rebased after it. It should be grouped in "hostdevsubsysscsi", to make sure it's only valid for SCSI device when doing RNG validation.

<ref name="deviceBoot"/> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f3fca7f..d9d6b9f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3452,8 +3452,8 @@ error: }

int -virDomainDiskFindControllerModel(virDomainDefPtr def, - virDomainDiskDefPtr disk, +virDomainDeviceFindControllerModel(virDomainDefPtr def, + virDomainDeviceInfoPtr info, int controllerType) { int model = -1; @@ -3461,7 +3461,7 @@ virDomainDiskFindControllerModel(virDomainDefPtr def,

for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type == controllerType && - def->controllers[i]->idx == disk->info.addr.drive.controller) + def->controllers[i]->idx == info->addr.drive.controller) model = def->controllers[i]->model; }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index edddf25..f8e3973 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -439,6 +439,8 @@ struct _virDomainHostdevDef { } source; virDomainHostdevOrigStates origstates; virDomainDeviceInfoPtr info; /* Guest address */ + /* readonly is only used for scsi hostdev */ + unsigned int readonly; };

/* Two types of disk backends */ @@ -1981,8 +1983,8 @@ void virDomainInputDefFree(virDomainInputDefPtr def); void virDomainDiskDefFree(virDomainDiskDefPtr def); void virDomainLeaseDefFree(virDomainLeaseDefPtr def); void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def); -int virDomainDiskFindControllerModel(virDomainDefPtr def, - virDomainDiskDefPtr disk, +int virDomainDeviceFindControllerModel(virDomainDefPtr def, + virDomainDeviceInfoPtr info, int controllerType); virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def, int bus, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 96eea0a..f2eefc3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -128,6 +128,7 @@ virDomainDeviceAddressTypeToString; virDomainDeviceDefCopy; virDomainDeviceDefFree; virDomainDeviceDefParse; +virDomainDeviceFindControllerModel; virDomainDeviceInfoCopy; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; @@ -145,7 +146,6 @@ virDomainDiskDeviceTypeToString; virDomainDiskErrorPolicyTypeFromString; virDomainDiskErrorPolicyTypeToString; virDomainDiskFindByBusAndDst; -virDomainDiskFindControllerModel; virDomainDiskGeometryTransTypeFromString; virDomainDiskGeometryTransTypeToString; virDomainDiskHostDefFree; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a0c278f..eac72c2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -557,7 +557,7 @@ qemuAssignDeviceDiskAliasCustom(virDomainDefPtr def, if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { controllerModel = - virDomainDiskFindControllerModel(def, disk, + virDomainInfoFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI);

if ((qemuSetScsiControllerModel(def, qemuCaps, &controllerModel)) < 0) @@ -3179,7 +3179,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def, }

controllerModel = - virDomainDiskFindControllerModel(def, disk, + virDomainInfoFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI); if ((qemuSetScsiControllerModel(def, qemuCaps, &controllerModel)) < 0) goto error;

On 04/05/2013 07:17 PM, Paolo Bonzini wrote:

Il 03/04/2013 05:37, Osier Yang ha scritto:

...

<code>readonly</code> <dd>Indicates the device is readonly, only valid for SCSI device. <span class="since">Since 1.0.5</span>. </dd>

...

<dt><code>boot</code></dt> <dd>Specifies that the device is bootable. The<code>order</code> attribute determines the order in which devices will be tried during diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8d7e6db..ccf0913 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2911,6 +2911,11 @@ <ref name="alias"/> </optional> <optional> +<element name='readonly'> +<empty/> +</element> +</optional> +<optional>

Since the "readonly" is only valid for SCSI device. This patch should be either merged into 3/10, or rebased after it. It should be grouped in "hostdevsubsysscsi", to make sure it's only valid for SCSI device when doing RNG validation.

Note that sgio="filtered"/"unfiltered"/"default" can also be a valid attribute.

Oh, I see. Could we make it be done in another patch? Cheng

On 01/04/13 20:00, Han Cheng wrote:

The definiton of scsi adapter in storagespool.rng can be used by scsi hostdev. Split storagepool.rng into storagepool.rng and storagepoolcommon.rng and make domaincomman.rng include the latter one.

Don't think it worth having a separate file, IMHO just moving the scsi adapter definition into basictypes.rng is enough. And on the other hand, you have to modify docs/schemas/Makefile.am if going this way.

PortNumber is both defined in domaincomman.rng and storagespool.rng,

s/is both defined in/is defined in both/, s/domaincomman/domaincommon/

simplify it.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- docs/schemas/domaincommon.rng | 6 +----- docs/schemas/storagepool.rng | 18 ++---------------- docs/schemas/storagepoolcommon.rng | 21 +++++++++++++++++++++ 3 files changed, 24 insertions(+), 21 deletions(-) create mode 100644 docs/schemas/storagepoolcommon.rng

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ccf0913..364abf1 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3,6 +3,7 @@ <!-- domain-related definitions used in multiple grammars --> <include href='basictypes.rng'/> <include href='storageencryption.rng'/> + <include href='storagepoolcommon.rng'/> <include href='networkcommon.rng'/>

<!-- @@ -3815,11 +3816,6 @@ <param name="minInclusive">-1</param> </data> </define> - <define name="PortNumber"> - <data type="short"> - <param name="minInclusive">-1</param> - </data> - </define> <!-- weight currently is in range [100, 1000] --> <define name="weight"> <data type="unsignedInt"> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 0cc0406..8265eff 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -3,11 +3,12 @@ <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> <include href='basictypes.rng'/> + <include href='storagepoolcommon.rng'/> + <start> <ref name='pool'/> </start>

- <define name='pool'> <element name='pool'> <choice> @@ -274,15 +275,6 @@ </element> </define>

- <define name='sourceinfoadapter'> - <element name='adapter'> - <attribute name='name'> - <text/> - </attribute> - <empty/> - </element> - </define> - <define name='sourceinfoname'> <element name='name'> <text/> @@ -525,10 +517,4 @@ </data> </define>

- <define name="PortNumber"> - <data type="short"> - <param name="minInclusive">-1</param> - </data> - </define> - </grammar> diff --git a/docs/schemas/storagepoolcommon.rng b/docs/schemas/storagepoolcommon.rng new file mode 100644 index 0000000..200855e --- /dev/null +++ b/docs/schemas/storagepoolcommon.rng @@ -0,0 +1,21 @@ +<?xml version="1.0"?> +<!-- A Relax NG schema for the libvirt storage pool XML format --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" + datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + + <define name="PortNumber"> + <data type="short"> + <param name="minInclusive">-1</param> + </data> + </define> + + <define name='sourceinfoadapter'> + <element name='adapter'> + <attribute name='name'> + <text/> + </attribute> + <empty/> + </element> + </define> + +</grammar>

On Mon, Apr 01, 2013 at 08:00:55PM +0800, Han Cheng wrote:

Adding scsi hostdev, it should like:

<hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='0' unit='0'/> </source> <address type='drive' controller='0' bus='0' target='4' unit='8'/> </hostdev>

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- docs/formatdomain.html.in | 34 ++++++-- docs/schemas/domaincommon.rng | 24 +++++ src/conf/domain_audit.c | 10 ++ src/conf/domain_conf.c | 192 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 7 ++ 5 files changed, 258 insertions(+), 9 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a6bacfa..be3630e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2172,13 +2172,13 @@

<h4><a name="elementsHostDev">Host device assignment</a></h4>

- <h5><a href="elementsHostDevSubsys">USB / PCI devices</a></h5> + <h5><a href="elementsHostDevSubsys">USB / PCI / SCSI devices</a></h5>

<p> - USB and PCI devices attached to the host can be passed through + USB, PCI and SCSI devices attached to the host can be passed through to the guest using the <code>hostdev</code> element. - <span class="since">since after 0.4.4 for USB and 0.6.0 for PCI - (KVM only)</span>: + <span class="since">since after 0.4.4 for USB, 0.6.0 for PCI(KVM only) + and 0.13.0 for SCSI(KVM only)</span>:

s/0.13.0/1.0.5/

</p>

<pre> @@ -2209,12 +2209,29 @@ </devices> ...</pre>

+ <p>or:</p> + +<pre> + ... + <devices> + <hostdev mode='subsystem' type='scsi'> + <source> + <adapter name='scsi_host0'/> + <address type='scsi' bus='0' target='0' unit='0'/> + </source> + <readonly/> + <address type='scsi' controller= '0' bus='0' target='0' unit='0'/>

s/controller= '0'/controller='0'/

+ </hostdev> + </devices> + ...</pre> + <dl> <dt><code>hostdev</code></dt> <dd>The <code>hostdev</code> element is the main container for describing host devices. For usb device passthrough <code>mode</code> is always - "subsystem" and <code>type</code> is "usb" for a USB device and "pci" - for a PCI device. When <code>managed</code> is "yes" for a PCI + "subsystem" and <code>type</code> is "usb" for a USB device, "pci"

s/is "usb"/is "usb"/

+ for a PCI device and "scsi" for a SCSI device. When + <code>managed</code> is "yes" for a PCI device, it is detached from the host before being passed on to the guest, and reattached to the host after the guest exits. If <code>managed</code> is omitted or "no", and for USB @@ -2224,13 +2241,15 @@ hot-plugging the device, and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or stopping the - guest.</dd> + guest.For SCSI device, user is responsible to make sure do not

s/guest.For/guest. For/

+ use this device on host</dd> <dt><code>source</code></dt> <dd>The source element describes the device as seen from the host. The USB device can either be addressed by vendor / product id using the <code>vendor</code> and <code>product</code> elements or by the device's address on the hosts using the <code>address</code> element. PCI devices on the other hand can only be described by their <code>address</code>. + SCSI devices is described by <code>adapter<code> and <code>address<code>.

<code>adapter</code> and <code>address</code>, note the closing </code>.

<span class="since">Since 1.0.0</span>, the <code>source</code> element of USB devices may contain <code>startupPolicy</code> attribute which can @@ -2268,6 +2287,7 @@ <a href="#elementsOSBIOS">BIOS bootloader</a> section. <span class="since">Since 0.8.8</span> for PCI devices, <span class="since">Since 1.0.1</span> for USB devices. + <span class="since">Since 1.0.0</span> for SCSI devices.

s/1.0.0/1.0.5/

<dt><code>rom</code></dt> <dd>The <code>rom</code> element is used to change how a PCI device's ROM is presented to the guest. The optional <code>bar</code> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 364abf1..209d4f5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2945,6 +2945,7 @@ <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> + <ref name="hostdevsubsysscsi"/> </choice> </define>

@@ -2997,6 +2998,18 @@ </element> </define>

+ <define name="hostdevsubsysscsi"> + <attribute name="type"> + <value>scsi</value> + </attribute> + <element name="source"> + <ref name="sourceinfoadapter"/> + <element name="address"> + <ref name="scsiaddress"/> + </element> + </element> + </define> + <define name="hostdevcapsstorage"> <attribute name="type"> <value>storage</value> @@ -3041,6 +3054,17 @@ </attribute> </element> </define> + <define name="scsiaddress"> + <attribute name="bus"> + <ref name="driveBus"/> + </attribute> + <attribute name="target"> + <ref name="driveTarget"/> + </attribute> + <attribute name="unit"> + <ref name="driveUnit"/> + </attribute> + </define> <define name="usbportaddress"> <attribute name="bus"> <ref name="usbAddr"/> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index a776058..2fb5989 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -398,6 +398,16 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, goto cleanup; } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virAsprintf(&address, "%s:%d:%d:%d", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit) < 0) { + VIR_WARN("OOM while encoding audit message");

virReportOOMError();

+ goto cleanup; + } + 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 d9d6b9f..c98ca48 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -573,7 +573,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,

VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, "usb", - "pci") + "pci", + "scsi")

VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST, "storage", @@ -1558,7 +1559,8 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) if (def->parent.type == VIR_DOMAIN_DEVICE_NONE) virDomainDeviceInfoFree(def->info);

- if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { + switch (def->mode) { + case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: switch (def->source.caps.type) { case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: VIR_FREE(def->source.caps.u.storage.block); @@ -1567,6 +1569,11 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) VIR_FREE(def->source.caps.u.misc.chardev); break; } + break; + case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + VIR_FREE(def->source.subsys.u.scsi.adapter); + break; } }

@@ -3080,6 +3087,104 @@ virDomainParseLegacyDeviceAddress(char *devaddr, }

static int +virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node, + virDomainHostdevDefPtr def) +{ + int ret = -1, got_address = 0, got_adapter = 0; + xmlNodePtr cur; + char *bus = NULL, *target = NULL, *unit = NULL; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "address")) { + if (got_address) { + virReportError(VIR_ERR_XML_ERROR, + _("more than one addresses are specified")); + goto out; + } + if (!(bus = virXMLPropString(cur, "bus"))) { + virReportError(VIR_ERR_XML_ERROR, + _("bus must be specified for scsi hostdev address")); + goto out; + } + if (virStrToLong_ui(bus, NULL, 0, &def->source.subsys.u.scsi.bus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse bus %s"), bus); + goto out; + } + if (!(target = virXMLPropString(cur, "target"))) { + virReportError(VIR_ERR_XML_ERROR, + _("target must be specified for scsi hostdev address")); + goto out; + } + if (virStrToLong_ui(target, NULL, 0, &def->source.subsys.u.scsi.target) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse target %s"), target); + goto out; + } + if (!(unit = virXMLPropString(cur, "unit"))) { + virReportError(VIR_ERR_XML_ERROR, + _("unit must be specified for scsi hostdev address")); + goto out; + } + if (virStrToLong_ui(unit, NULL, 0, &def->source.subsys.u.scsi.unit) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse unit %s"), unit); + goto out; + } + VIR_FREE(bus); + VIR_FREE(target); + VIR_FREE(unit); + got_address = 1; + } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) { + if (got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + _("more than one adapters are specified")); + goto out; + } + if (!(def->source.subsys.u.scsi.adapter = + virXMLPropString(cur, "name"))) { + virReportError(VIR_ERR_XML_ERROR, + _("adapter must be specified for scsi hostdev address")); + goto out; + } + got_adapter = 1; + } else { + virReportError(VIR_ERR_XML_ERROR, + _("unknown scsi source type '%s'"), + cur->name); + goto out; + } + } + cur = cur->next; + } + + if (!got_address && !got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing adapter and address")); + goto out; + } + if (!got_address && got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing address")); + goto out; + } + if (got_address && !got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing adapter")); + goto out; + } + + ret = 0; +out: + VIR_FREE(bus); + VIR_FREE(target); + VIR_FREE(unit); + return ret; +} + +static int virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, virDomainHostdevDefPtr def) { @@ -3374,6 +3479,10 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if (virDomainHostdevSubsysUsbDefParseXML(sourcenode, def) < 0) goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virDomainHostdevSubsysScsiDefParseXML(sourcenode, def) < 0) + goto error; + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("address type='%s' not supported in hostdev interfaces"), @@ -8036,6 +8145,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, }

if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + xmlNodePtr cur; + unsigned int readonly = 0; switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && @@ -8044,6 +8155,20 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, _("PCI host devices must use 'pci' address type")); goto error; } + + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (readonly == 0 && + xmlStrEqual(cur->name, BAD_CAST "readonly")) + readonly = 1; + } + cur = cur->next; + } + def->readonly = readonly; + break; } } @@ -8567,6 +8692,17 @@ virDomainHostdevMatchSubsysPCI(virDomainHostdevDefPtr a, return 0; }

+static int +virDomainHostdevMatchSubsysSCSI(virDomainHostdevDefPtr a, + virDomainHostdevDefPtr b) +{ + if (STREQ(a->source.subsys.u.scsi.adapter, b->source.subsys.u.scsi.adapter) && + a->source.subsys.u.scsi.bus == b->source.subsys.u.scsi.bus && + a->source.subsys.u.scsi.target == b->source.subsys.u.scsi.target && + a->source.subsys.u.scsi.unit == b->source.subsys.u.scsi.unit) + return 1; + return 0; +}

static int virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, @@ -8580,6 +8716,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, return virDomainHostdevMatchSubsysPCI(a, b); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: return virDomainHostdevMatchSubsysUSB(a, b); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + return virDomainHostdevMatchSubsysSCSI(a, b); } return 0; } @@ -10773,6 +10911,16 @@ virDomainDefParseXML(virCapsPtr caps, goto error; }

+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + /* reverse first 16 unit for disk usage */ + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + hostdev->info->addr.drive.controller = 0; + hostdev->info->addr.drive.bus = 0; + hostdev->info->addr.drive.target = 0; + hostdev->info->addr.drive.unit = 16 + i; + } + def->hostdevs[def->nhostdevs++] = hostdev; } VIR_FREE(nodes); @@ -12340,6 +12488,30 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def) return 0; }

+static int +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) +{ + /* Look for any hostdev scsi dev */ + int i; + int maxController = -1; + virDomainHostdevDefPtr hostdev; + + for (i = 0; i < def->nhostdevs; i++) { + hostdev = *(def->hostdevs + i); + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + (int)hostdev->info->addr.drive.controller > maxController) { + maxController = hostdev->info->addr.drive.controller; + } + } + + for (i = 0; i <= maxController; i++) { + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i) < 0) + return -1; + } + + return 0; +}

/* * Based on the declared <address/> info for any devices, @@ -12376,6 +12548,9 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) if (virDomainDefMaybeAddSmartcardController(def) < 0) return -1;

+ if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + return -1; + return 0; }

@@ -13194,6 +13369,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAdjustIndent(buf, 2); switch (def->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + virBufferAsprintf(buf, "<adapter name='%s'/>\n", + def->source.subsys.u.scsi.adapter); + virBufferAsprintf(buf, "<address %sbus='%d' target='%d' unit='%d'/>\n", + includeTypeInAddr ? "type='scsi' " : "", + def->source.subsys.u.scsi.bus, + def->source.subsys.u.scsi.target, + def->source.subsys.u.scsi.unit); + break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: if (def->source.subsys.u.usb.vendor) { virBufferAsprintf(buf, "<vendor id='0x%.4x'/>\n", @@ -14446,6 +14630,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: if (virDomainHostdevDefFormatSubsys(buf, def, flags, false) < 0) return -1; + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + def->readonly) + virBufferAsprintf(buf, "<readonly/>\n"); break; case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: if (virDomainHostdevDefFormatCaps(buf, def) < 0) @@ -14454,6 +14641,7 @@ virDomainHostdevDefFormat(virBufferPtr buf, } virBufferAdjustIndent(buf, -6);

+ if (virDomainDeviceInfoFormat(buf, def->info, flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f8e3973..2d238eb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -380,6 +380,7 @@ enum virDomainHostdevMode { enum virDomainHostdevSubsysType { VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,

VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST }; @@ -399,6 +400,12 @@ struct _virDomainHostdevSubsys { unsigned vendor; unsigned product; } usb; + struct { + char *adapter; + unsigned bus; + unsigned target; + unsigned unit; + } scsi; virDevicePCIAddress pci; /* host address */ } u; }; -- 1.7.1

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

On Fri, Apr 05, 2013 at 01:00:00PM +0800, Han Cheng wrote:

On 04/02/2013 11:19 AM, Hu Tao wrote:

...

On Mon, Apr 01, 2013 at 08:00:55PM +0800, Han Cheng wrote:

...

diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index a776058..2fb5989 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -398,6 +398,16 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, goto cleanup; } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virAsprintf(&address, "%s:%d:%d:%d", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit)< 0) { + VIR_WARN("OOM while encoding audit message");

virReportOOMError();

I'm not sure this is good. Other functions in this file use VIR_WARN to report OOM. If we change this place, we should change all others for consistence. Besides, Michal Privoznik is try to drop almost all virReportOOMError.

Yep, you are correctin what youoriginally had. The domain_audit.c file is special in that it does *not* use virReport*Error, only VIR_WARN, becasue we don't want to treat audit log failure as fatal to guest startup. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 01/04/13 20:00, Han Cheng wrote:

Adding scsi hostdev, it should like:

s/Adding/Add, s/should/looks/,

<hostdev mode='subsystem' type='scsi'> <source> <adapter name='scsi_host0'/> <address bus='0' target='0' unit='0'/> </source> <address type='drive' controller='0' bus='0' target='4' unit='8'/> </hostdev>

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- docs/formatdomain.html.in | 34 ++++++-- docs/schemas/domaincommon.rng | 24 +++++ src/conf/domain_audit.c | 10 ++ src/conf/domain_conf.c | 192 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 7 ++ 5 files changed, 258 insertions(+), 9 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a6bacfa..be3630e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2172,13 +2172,13 @@

<h4><a name="elementsHostDev">Host device assignment</a></h4>

- <h5><a href="elementsHostDevSubsys">USB / PCI devices</a></h5> + <h5><a href="elementsHostDevSubsys">USB / PCI / SCSI devices</a></h5>

<p> - USB and PCI devices attached to the host can be passed through + USB, PCI and SCSI devices attached to the host can be passed through to the guest using the <code>hostdev</code> element. - <span class="since">since after 0.4.4 for USB and 0.6.0 for PCI - (KVM only)</span>: + <span class="since">since after 0.4.4 for USB, 0.6.0 for PCI(KVM only) + and 0.13.0 for SCSI(KVM only)</span>:

s/0.13.0/1.0.5/, confused with qemu's version?

</p>

<pre> @@ -2209,12 +2209,29 @@ </devices> ...</pre>

+ <p>or:</p> + +<pre> + ... + <devices> + <hostdev mode='subsystem' type='scsi'> + <source> + <adapter name='scsi_host0'/> + <address type='scsi' bus='0' target='0' unit='0'/> + </source> + <readonly/> + <address type='scsi' controller= '0' bus='0' target='0' unit='0'/> + </hostdev> + </devices> + ...</pre> + <dl> <dt><code>hostdev</code></dt> <dd>The <code>hostdev</code> element is the main container for describing host devices. For usb device passthrough <code>mode</code> is always - "subsystem" and <code>type</code> is "usb" for a USB device and "pci" - for a PCI device. When <code>managed</code> is "yes" for a PCI + "subsystem" and <code>type</code> is "usb" for a USB device, "pci" + for a PCI device and "scsi" for a SCSI device. When + <code>managed</code> is "yes" for a PCI device, it is detached from the host before being passed on to the guest, and reattached to the host after the guest exits. If <code>managed</code> is omitted or "no", and for USB @@ -2224,13 +2241,15 @@ hot-plugging the device, and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or stopping the - guest.</dd> + guest.For SCSI device, user is responsible to make sure do not + use this device on host</dd>

s/do not use this device on host/the device is not used by either host or any guest./

<dt><code>source</code></dt> <dd>The source element describes the device as seen from the host. The USB device can either be addressed by vendor / product id using the <code>vendor</code> and <code>product</code> elements or by the device's address on the hosts using the <code>address</code> element. PCI devices on the other hand can only be described by their <code>address</code>. + SCSI devices is described by <code>adapter<code> and <code>address<code>.

<span class="since">Since 1.0.0</span>, the <code>source</code> element of USB devices may contain <code>startupPolicy</code> attribute which can @@ -2268,6 +2287,7 @@ <a href="#elementsOSBIOS">BIOS bootloader</a> section. <span class="since">Since 0.8.8</span> for PCI devices, <span class="since">Since 1.0.1</span> for USB devices. + <span class="since">Since 1.0.0</span> for SCSI devices.

s/1.0.0/1.0.5/

<dt><code>rom</code></dt> <dd>The <code>rom</code> element is used to change how a PCI device's ROM is presented to the guest. The optional <code>bar</code> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 364abf1..209d4f5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2945,6 +2945,7 @@ <choice> <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> + <ref name="hostdevsubsysscsi"/> </choice> </define>

@@ -2997,6 +2998,18 @@ </element> </define>

+ <define name="hostdevsubsysscsi"> + <attribute name="type"> + <value>scsi</value> + </attribute> + <element name="source"> + <ref name="sourceinfoadapter"/> + <element name="address"> + <ref name="scsiaddress"/> + </element> + </element> + </define>

I was confused. The attribute "readonly" should be in "hostdevsubsysscsi", as it's only about the "source".

+ <define name="hostdevcapsstorage"> <attribute name="type"> <value>storage</value> @@ -3041,6 +3054,17 @@ </attribute> </element> </define> + <define name="scsiaddress"> + <attribute name="bus"> + <ref name="driveBus"/> + </attribute> + <attribute name="target"> + <ref name="driveTarget"/> + </attribute> + <attribute name="unit"> + <ref name="driveUnit"/> + </attribute> + </define> <define name="usbportaddress"> <attribute name="bus"> <ref name="usbAddr"/> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index a776058..2fb5989 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -398,6 +398,16 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, goto cleanup; } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virAsprintf(&address, "%s:%d:%d:%d", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit) < 0) { + VIR_WARN("OOM while encoding audit message"); + goto cleanup; + } + 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 d9d6b9f..c98ca48 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -573,7 +573,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,

VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, "usb", - "pci") + "pci", + "scsi")

VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST, "storage", @@ -1558,7 +1559,8 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) if (def->parent.type == VIR_DOMAIN_DEVICE_NONE) virDomainDeviceInfoFree(def->info);

- if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) { + switch (def->mode) { + case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: switch (def->source.caps.type) { case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE: VIR_FREE(def->source.caps.u.storage.block); @@ -1567,6 +1569,11 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) VIR_FREE(def->source.caps.u.misc.chardev); break; } + break; + case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + VIR_FREE(def->source.subsys.u.scsi.adapter); + break; } }

@@ -3080,6 +3087,104 @@ virDomainParseLegacyDeviceAddress(char *devaddr, }

static int +virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node, + virDomainHostdevDefPtr def) +{ + int ret = -1, got_address = 0, got_adapter = 0;

bool got_address = false, got_adapter = false;

+ xmlNodePtr cur; + char *bus = NULL, *target = NULL, *unit = NULL; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "address")) { + if (got_address) { + virReportError(VIR_ERR_XML_ERROR, + _("more than one addresses are specified")); + goto out; + } + if (!(bus = virXMLPropString(cur, "bus"))) { + virReportError(VIR_ERR_XML_ERROR, + _("bus must be specified for scsi hostdev address")); + goto out; + } + if (virStrToLong_ui(bus, NULL, 0, &def->source.subsys.u.scsi.bus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse bus %s"), bus); + goto out; + } + if (!(target = virXMLPropString(cur, "target"))) { + virReportError(VIR_ERR_XML_ERROR, + _("target must be specified for scsi hostdev address")); + goto out; + } + if (virStrToLong_ui(target, NULL, 0, &def->source.subsys.u.scsi.target) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse target %s"), target); + goto out; + } + if (!(unit = virXMLPropString(cur, "unit"))) { + virReportError(VIR_ERR_XML_ERROR, + _("unit must be specified for scsi hostdev address")); + goto out; + } + if (virStrToLong_ui(unit, NULL, 0, &def->source.subsys.u.scsi.unit) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse unit %s"), unit); + goto out; + } + VIR_FREE(bus); + VIR_FREE(target); + VIR_FREE(unit);

No need to free these 3 strings here. They are free'ed in "out:" anyway. And since you have "git_address". They can be only parsed once. So no worry of the leaking here.

+ got_address = 1;

And got_adress = true;

+ } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) { + if (got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + _("more than one adapters are specified")); + goto out; + } + if (!(def->source.subsys.u.scsi.adapter = + virXMLPropString(cur, "name"))) { + virReportError(VIR_ERR_XML_ERROR, + _("adapter must be specified for scsi hostdev address")); + goto out; + } + got_adapter = 1;

got_adapter = true;

+ } else { + virReportError(VIR_ERR_XML_ERROR, + _("unknown scsi source type '%s'"),

Improper error.

+ cur->name); + goto out; + } + } + cur = cur->next; + } + + if (!got_address && !got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing adapter and address")); + goto out; + } + if (!got_address && got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing address")); + goto out; + } + if (got_address && !got_adapter) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing adapter")); + goto out; + }

Above checking can be simplied as: if (!got_address || !got_dapter) { virReportError(VIR_ERR_XML_ERROR, "%s", _("'adapter' and 'address' must be specified")); goto out; }

+ + ret = 0; +out:

s/out/cleanup/, this applies across the function. Some old code still use the "out", but new code should be consistent as much as possible.

+ VIR_FREE(bus); + VIR_FREE(target); + VIR_FREE(unit); + return ret; +} + +static int virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, virDomainHostdevDefPtr def) { @@ -3374,6 +3479,10 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, if (virDomainHostdevSubsysUsbDefParseXML(sourcenode, def) < 0) goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (virDomainHostdevSubsysScsiDefParseXML(sourcenode, def) < 0) + goto error; + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("address type='%s' not supported in hostdev interfaces"), @@ -8036,6 +8145,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, }

if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + xmlNodePtr cur; + unsigned int readonly = 0; switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && @@ -8044,6 +8155,20 @@ (const xmlNodePtr node, _("PCI host devices must use 'pci' address type")); goto error; } + + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (readonly == 0 && + xmlStrEqual(cur->name, BAD_CAST "readonly")) + readonly = 1; + } + cur = cur->next; + } + def->readonly = readonly;

Agreed with Hu Tao, that "readonly" should be of boolean type.

+ break; } } @@ -8567,6 +8692,17 @@ virDomainHostdevMatchSubsysPCI(virDomainHostdevDefPtr a, return 0; }

+static int +virDomainHostdevMatchSubsysSCSI(virDomainHostdevDefPtr a, + virDomainHostdevDefPtr b) +{ + if (STREQ(a->source.subsys.u.scsi.adapter, b->source.subsys.u.scsi.adapter) && + a->source.subsys.u.scsi.bus == b->source.subsys.u.scsi.bus && + a->source.subsys.u.scsi.target == b->source.subsys.u.scsi.target && + a->source.subsys.u.scsi.unit == b->source.subsys.u.scsi.unit) + return 1; + return 0; +}

static int virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, @@ -8580,6 +8716,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, return virDomainHostdevMatchSubsysPCI(a, b); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: return virDomainHostdevMatchSubsysUSB(a, b); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + return virDomainHostdevMatchSubsysSCSI(a, b); } return 0; } @@ -10773,6 +10911,16 @@ virDomainDefParseXML(virCapsPtr caps, goto error; }

+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + /* reverse first 16 unit for disk usage */ + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + hostdev->info->addr.drive.controller = 0; + hostdev->info->addr.drive.bus = 0; + hostdev->info->addr.drive.target = 0;

Why this defdaults to 0? Can you explain it either in the commit log or by comments?

+ hostdev->info->addr.drive.unit = 16 + i;

And why the "16".

+ } + def->hostdevs[def->nhostdevs++] = hostdev; } VIR_FREE(nodes); @@ -12340,6 +12488,30 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def) return 0; }

+static int +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)

Why do we need this?

+{ + /* Look for any hostdev scsi dev */ + int i; + int maxController = -1; + virDomainHostdevDefPtr hostdev; + + for (i = 0; i < def->nhostdevs; i++) { + hostdev = *(def->hostdevs + i);

Generally we use: hostdev = def->hostdevs[i];

+ if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + (int)hostdev->info->addr.drive.controller > maxController) { + maxController = hostdev->info->addr.drive.controller; + } + } + + for (i = 0; i <= maxController; i++) { + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i) < 0) + return -1; + } + + return 0; +}

/* * Based on the declared <address/> info for any devices, @@ -12376,6 +12548,9 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def) if (virDomainDefMaybeAddSmartcardController(def) < 0) return -1;

+ if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + return -1; + return 0; }

@@ -13194,6 +13369,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, virBufferAdjustIndent(buf, 2); switch (def->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + virBufferAsprintf(buf, "<adapter name='%s'/>\n", + def->source.subsys.u.scsi.adapter); + virBufferAsprintf(buf, "<address %sbus='%d' target='%d' unit='%d'/>\n", + ? "type='scsi' " : "", + def->source.subsys.u.scsi.bus, + def->source.subsys.u.scsi.target, + def->source.subsys.u.scsi.unit); + break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: if (def->source.subsys.u.usb.vendor) { virBufferAsprintf(buf, "<vendor id='0x%.4x'/>\n", @@ -14446,6 +14630,9 @@ virDomainHostdevDefFormat(virBufferPtr buf, case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: if (virDomainHostdevDefFormatSubsys(buf, def, flags, false) < 0) return -1; + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + def->readonly) + virBufferAsprintf(buf, "<readonly/>\n"); break; case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES: if (virDomainHostdevDefFormatCaps(buf, def) < 0) @@ -14454,6 +14641,7 @@ virDomainHostdevDefFormat(virBufferPtr buf, } virBufferAdjustIndent(buf, -6);

+ if (virDomainDeviceInfoFormat(buf, def->info, flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f8e3973..2d238eb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -380,6 +380,7 @@ enum virDomainHostdevMode { enum virDomainHostdevSubsysType { VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,

VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST }; @@ -399,6 +400,12 @@ struct _virDomainHostdevSubsys { unsigned vendor; unsigned product; } usb; + struct { + char *adapter; + unsigned bus; + unsigned target; + unsigned unit; + } scsi; virDevicePCIAddress pci; /* host address */ } u; };

On 04/03/2013 04:32 PM, Osier Yang wrote:

On 03/04/13 16:29, Osier Yang wrote:

...

On 01/04/13 20:00, Han Cheng wrote:

...

<dt><code>hostdev</code></dt> <dd>The <code>hostdev</code> element is the main container for describing host devices. For usb device passthrough <code>mode</code> is always - "subsystem" and <code>type</code> is "usb" for a USB device and "pci" - for a PCI device. When <code>managed</code> is "yes" for a PCI + "subsystem" and <code>type</code> is "usb" for a USB device, "pci" + for a PCI device and "scsi" for a SCSI device. When + <code>managed</code> is "yes" for a PCI device, it is detached from the host before being passed on to the guest, and reattached to the host after the guest exits. If <code>managed</code> is omitted or "no", and for USB @@ -2224,13 +2241,15 @@ hot-plugging the device, and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or stopping the - guest.</dd> + guest.For SCSI device, user is responsible to make sure do not + use this device on host</dd>

s/do not use this device on host/the device is not used by either host or any guest./

I think host only. Guest is in our controll as 7/10.

On 04/03/2013 11:41 AM, Osier Yang wrote:

On 01/04/13 20:00, Han Cheng wrote:

...

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7101f67..a3d8595 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -174,6 +174,8 @@ enum virQEMUCapsFlags { QEMU_CAPS_VIRTIO_CCW = 132, /* -device virtio-*-ccw */ QEMU_CAPS_DTB = 133, /* -dtb file */ QEMU_CAPS_SCSI_MEGASAS = 134, /* -device megasas */ + QEMU_CAPS_SCSI_GENERIC = 135, /* -device scsi-generic */ + QEMU_CAPS_SCSI_GENERIC_BOOTINDEX = 136, /* scsi-generic.bootindex */ QEMU_CAPS_LAST, /* this must always be the last item */ };

Looks good, but don't you need to update qemuhelptestdata to make sure the "make check" is not broken? Yes, these changes were made in 10/10.

Cheng

On 04/03/2013 11:51 AM, Osier Yang wrote:

On 03/04/13 11:48, Han Cheng wrote:

...

On 04/03/2013 11:41 AM, Osier Yang wrote:

...

On 01/04/13 20:00, Han Cheng wrote:

...

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7101f67..a3d8595 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -174,6 +174,8 @@ enum virQEMUCapsFlags { QEMU_CAPS_VIRTIO_CCW = 132, /* -device virtio-*-ccw */ QEMU_CAPS_DTB = 133, /* -dtb file */ QEMU_CAPS_SCSI_MEGASAS = 134, /* -device megasas */ + QEMU_CAPS_SCSI_GENERIC = 135, /* -device scsi-generic */ + QEMU_CAPS_SCSI_GENERIC_BOOTINDEX = 136, /* scsi-generic.bootindex */ QEMU_CAPS_LAST, /* this must always be the last item */ };

Looks good, but don't you need to update qemuhelptestdata to make sure the "make check" is not broken? Yes, these changes were made in 10/10.

It has to be be in this patch, every single patch should *not* break the build.

Actually, this doesn't break this as we place both changes on qemuhelptestdata and qemuhelptest.c in 10/10. Or, shall we move these from 10/10 into this? Cheng

On 01/04/13 20:00, Han Cheng wrote:

This patch add util functions for scsi hostdev.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 22 +++ src/util/virscsi.c | 399 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virscsi.h | 83 ++++++++++ 5 files changed, 506 insertions(+), 0 deletions(-) create mode 100644 src/util/virscsi.c create mode 100644 src/util/virscsi.h

diff --git a/po/POTFILES.in b/po/POTFILES.in index 91e5c02..39a0a19 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -174,6 +174,7 @@ src/util/virportallocator.c src/util/virprocess.c src/util/virrandom.c src/util/virsexpr.c +src/util/virscsi.c src/util/virsocketaddr.c src/util/virstatslinux.c src/util/virstoragefile.c diff --git a/src/Makefile.am b/src/Makefile.am index 3f69d39..49d7f88 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -111,6 +111,7 @@ UTIL_SOURCES = \ util/virportallocator.c util/virportallocator.h \ util/virprocess.c util/virprocess.h \ util/virrandom.h util/virrandom.c \ + util/virscsi.c util/virscsi.h \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstatslinux.c util/virstatslinux.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f2eefc3..6a5962e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1665,6 +1665,28 @@ virRandomGenerateWWN; virRandomInt;

+# util/virscsi.h +virSCSIDeviceFileIterate; +virSCSIDeviceFree; +virSCSIDeviceGetAdapter; +virSCSIDeviceGetBus; +virSCSIDeviceGetDevStr; +virSCSIDeviceGetName; +virSCSIDeviceGetReadonly; +virSCSIDeviceGetTarget; +virSCSIDeviceGetUnit; +virSCSIDeviceGetUsedBy; +virSCSIDeviceListAdd; +virSCSIDeviceListCount; +virSCSIDeviceListDel; +virSCSIDeviceListFind; +virSCSIDeviceListGet; +virSCSIDeviceListNew; +virSCSIDeviceListSteal; +virSCSIDeviceNew; +virSCSIDeviceSetUsedBy; + + # util/virsexpr.h sexpr2string; sexpr_append; diff --git a/src/util/virscsi.c b/src/util/virscsi.c new file mode 100644 index 0000000..5d0dcd7 --- /dev/null +++ b/src/util/virscsi.c @@ -0,0 +1,399 @@ +/* + * virscsi.c: helper APIs for managing host SCSI devices + * + * Copyright (C) 2013 Fujitsu, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Han Cheng <hanc.fnst@cn.fujitsu.com> + */ + +#include <config.h> + +#include <dirent.h> +#include <fcntl.h> +#include <inttypes.h> +#include <limits.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include "virscsi.h" +#include "virlog.h" +#include "viralloc.h" +#include "virutil.h" +#include "virerror.h" + +#define SCSI_DEVFS "/sys/bus/scsi/devices"

Generally we name macro like this as "SYSFS_SCSI_DEVICES". virpci.c is too old.

+ +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +struct _virSCSIDevice { + unsigned int adapter; + unsigned int bus; + unsigned int target; + unsigned int unit; + + char *name; /* adapter:bus:target:unit */ + char *id; /* model vendor */ + char *path; + const char *used_by; /* name of the domain using this dev */ + + unsigned int readonly : 1; +}; + +struct _virSCSIDeviceList { + virObjectLockable parent; + unsigned int count; + virSCSIDevicePtr *devs; +}; + +static virClassPtr virSCSIDeviceListClass; + +static void virSCSIDeviceListDispose(void *obj); + +static int virSCSIOnceInit(void) +{ + if (!(virSCSIDeviceListClass = virClassNew(virClassForObjectLockable(), + "virSCSIDeviceList", + sizeof(virSCSIDeviceList), + virSCSIDeviceListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSCSI) + +static int virSCSIDeviceGetAdapterId(char *adapter, unsigned int *adapterid) +{ + if (STRSKIP(adapter, "scsi_host") && + virStrToLong_ui(adapter + strlen("scsi_host"), NULL, 0, + adapterid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse adapter %s"), adapter); + return -1; + } + + return 0; +} + +char * +virSCSIDeviceGetDevStr(char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit)

Indentions?

+{ + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *sg = NULL; + unsigned int adapterid; + + if (virSCSIDeviceGetAdapterId(adapter, &adapterid) < 0) + goto out; + + if (virAsprintf(&path, + SCSI_DEVFS "/%d:%d:%d:%d/scsi_generic", + adapterid, bus, target, unit) < 0) { + virReportOOMError(); + goto out; + } + if (!(dir = opendir(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"), path); + goto out; + } + + while ((entry = readdir(dir))) { + if (entry->d_name[0] == '.') + continue; + + if (virAsprintf(&sg, "%s", entry->d_name) < 0) { + virReportOOMError(); + goto out; + } + } + +out: + closedir(dir); + VIR_FREE(path); + return sg; +} + +virSCSIDevicePtr +virSCSIDeviceNew(char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit, + unsigned int readonly) +{ + virSCSIDevicePtr dev; + char *sg = NULL; + char *vendor = NULL; + char *model = NULL; + char *tmp = NULL; + + if (VIR_ALLOC(dev) < 0) { + virReportOOMError(); + return NULL; + } + + dev->bus = bus; + dev->target = target; + dev->unit = unit; + dev->readonly = readonly; + + if (!(sg = virSCSIDeviceGetDevStr(adapter, bus, target, unit))) { + goto out; + }

if (!(sg = virSCSIDeviceGetDevStr(adapter, bus, target, unit))) goto out;

ve + if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) { + goto out; + }

Likewise.

v + if (virAsprintf(&dev->name, "%d:%d:%d:%d", + dev->adapter, dev->bus, dev->bus, + dev->unit) < 0) { + virReportOOMError(); + goto out; + } + if (virAsprintf(&dev->path, "/dev/%s", sg) < 0) { + virReportOOMError(); + goto out; + } + if (access(dev->path, F_OK) != 0) { + virReportSystemError(errno, + _("Device %s not found: could not access %s"), + dev->name, dev->path); + goto out; + } + if (virAsprintf(&tmp, SCSI_DEVFS "/%s/vendor", dev->name) < 0) { + virReportOOMError(); + goto out; + } + if (virFileReadAll(tmp, 1024, &vendor) < 0) + goto out; + VIR_FREE(tmp); + tmp = NULL; + if (virAsprintf(&tmp, SCSI_DEVFS "/%s/model", dev->name) < 0) { + virReportOOMError(); + goto out; + } + if (virFileReadAll(tmp, 1024, &model) < 0) + goto out; + *(vendor + strlen(vendor) - 1) = '\0';

This should be right after reading "vendor"

+ *(model + strlen(model) - 1) = '\0'; + if (virAsprintf(&dev->id, "%s %s", vendor, model) < 0) {

Is it possible for the "vendor" and "name" contains white space(s)? if it is, then separating them by one space has problem.

+ virReportOOMError(); + goto out; + } + VIR_FREE(tmp); + VIR_FREE(vendor); + VIR_FREE(model);

Redundant code, these 3 frees can be removed if you do as following....

+ + VIR_DEBUG("%s %s: initialized", dev->id, dev->name); + + return dev;

ret = dev;

+ +out: + VIR_FREE(tmp); + VIR_FREE(vendor); + VIR_FREE(model); + virSCSIDeviceFree(dev);

if (!ret) virSCSIDeviceFree(dev);

+ return NULL;

return ret;

+} + +void +virSCSIDeviceFree(virSCSIDevicePtr dev) +{ + if (!dev) + return; + VIR_DEBUG("%s %s: freeing", dev->id, dev->name); + VIR_FREE(dev->id); + VIR_FREE(dev->name); + VIR_FREE(dev->path); + VIR_FREE(dev); +} + + +void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, + const char *name) +{ + dev->used_by = name; +} + +const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev) +{ + return dev->used_by; +} + +const char *virSCSIDeviceGetName(virSCSIDevicePtr dev) +{ + return dev->name; +} + +unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev) +{ + return dev->adapter; +} + +unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev) +{ + return dev->bus; +} + +unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev) +{ + return dev->target; +} + +unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev) +{ + return dev->unit; +} + +unsigned int virSCSIDeviceGetReadonly(virSCSIDevicePtr dev) +{ + return dev->readonly; +} + +int virSCSIDeviceFileIterate(virSCSIDevicePtr dev, + virSCSIDeviceFileActor actor, + void *opaque) +{ + return (actor)(dev, dev->path, opaque); +} + +virSCSIDeviceListPtr +virSCSIDeviceListNew(void) +{ + virSCSIDeviceListPtr list; + + if (virSCSIInitialize() < 0) + return NULL; + + if (!(list = virObjectLockableNew(virSCSIDeviceListClass))) + return NULL; + + return list; +} + +static void +virSCSIDeviceListDispose(void *obj) +{ + virSCSIDeviceListPtr list = obj; + int i; + + for (i = 0; i < list->count; i++) + virSCSIDeviceFree(list->devs[i]); + + VIR_FREE(list->devs); +} + +int +virSCSIDeviceListAdd(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + if (virSCSIDeviceListFind(list, dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device %s is already in use"),

"Device %s already exists". It should be cut-n-paste from virpci.c.

+ dev->name); + return -1; + } + + if (VIR_REALLOC_N(list->devs, list->count+1) < 0) { + virReportOOMError(); + return -1; + } + + list->devs[list->count++] = dev; + + return 0; +} + +virSCSIDevicePtr +virSCSIDeviceListGet(virSCSIDeviceListPtr list, + int idx) +{ + if (idx >= list->count || + idx < 0) + return NULL; + + return list->devs[idx]; +} + +int +virSCSIDeviceListCount(virSCSIDeviceListPtr list) +{ + return list->count; +} + +virSCSIDevicePtr +virSCSIDeviceListSteal(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + virSCSIDevicePtr ret = NULL; + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->adapter != dev->adapter || + list->devs[i]->bus != dev->bus || + list->devs[i]->target != dev->target || + list->devs[i]->unit != dev->unit) + continue; + + ret = list->devs[i]; + + if (i != list->count--) + memmove(&list->devs[i], + &list->devs[i+1], + sizeof(*list->devs) * (list->count - i)); + + if (VIR_REALLOC_N(list->devs, list->count) < 0) { + ; /* not fatal */ + } + + break; + } + return ret; +} + +void +virSCSIDeviceListDel(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev); + virSCSIDeviceFree(ret); +} + +virSCSIDevicePtr +virSCSIDeviceListFind(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->adapter == dev->adapter && + list->devs[i]->bus == dev->bus && + list->devs[i]->target == dev->target && + list->devs[i]->unit == dev->unit) + return list->devs[i]; + } + + return NULL; +} diff --git a/src/util/virscsi.h b/src/util/virscsi.h new file mode 100644 index 0000000..fbf143c --- /dev/null +++ b/src/util/virscsi.h @@ -0,0 +1,83 @@ +/* + * virscsi.h: helper APIs for managing host SCSI devices + * + * Copyright (C) 2013 Fujitsu, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Han Cheng <hanc.fnst@cn.fujitsu.com> + */ + +#ifndef __VIR_SCSI_H__ +# define __VIR_SCSI_H__ + +# include "internal.h" +# include "virobject.h" + +typedef struct _virSCSIDevice virSCSIDevice; +typedef virSCSIDevice *virSCSIDevicePtr; +typedef struct _virSCSIDeviceList virSCSIDeviceList; +typedef virSCSIDeviceList *virSCSIDeviceListPtr; + +char *virSCSIDeviceGetDevStr(char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit); + +virSCSIDevicePtr virSCSIDeviceNew(char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit, + unsigned int readonly);

I'm wondering if we can abstract a common list object, at least we can reduce the redundant code in virpci.c, virusb.c, virscsi.c. But it can be future patch.

+ +void virSCSIDeviceFree(virSCSIDevicePtr dev); +void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name); +const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev); +const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetReadonly(virSCSIDevicePtr dev); + +/* + * Callback that will be invoked once for each file + * associated with / used for SCSI host device access. + * + * Should return 0 if successfully processed, or + * -1 to indicate error and abort iteration + */ +typedef int (*virSCSIDeviceFileActor)(virSCSIDevicePtr dev, + const char *path, void *opaque); + +int virSCSIDeviceFileIterate(virSCSIDevicePtr dev, + virSCSIDeviceFileActor actor, + void *opaque); + +virSCSIDeviceListPtr virSCSIDeviceListNew(void); +int virSCSIDeviceListAdd(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); +virSCSIDevicePtr virSCSIDeviceListGet(virSCSIDeviceListPtr list, + int idx); +int virSCSIDeviceListCount(virSCSIDeviceListPtr list); +virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); +void virSCSIDeviceListDel(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); +virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); + +#endif /* __VIR_SCSI_H__ */

On 01/04/13 20:00, Han Cheng wrote:

For scsi-generic, the command line will be like:

-drive file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0 \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0

The relationship between the libvirt address attrs and the qdev properties are(channel should always be 0): bus=scsi<controller>.0 scsi-id=<target> lun=<unit>

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_command.h | 6 ++ 2 files changed, 134 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eac72c2..e1af64f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -557,7 +557,7 @@ qemuAssignDeviceDiskAliasCustom(virDomainDefPtr def, if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { controllerModel = - virDomainInfoFindControllerModel(def, &disk->info, + virDomainDeviceFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI);

Indention.

if ((qemuSetScsiControllerModel(def, qemuCaps, &controllerModel)) < 0) @@ -659,7 +659,16 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev } }

- if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + if (virAsprintf(&hostdev->info->alias, "hostdev-%s-%d-%d-%d", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit) < 0) { + virReportOOMError(); + return -1; + } + } else if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { virReportOOMError(); return -1; } @@ -3179,7 +3188,7 @@ qemuBuildDriveDevStr(virDomainDefPtr def, }

controllerModel = - virDomainInfoFindControllerModel(def, &disk->info, + virDomainDeviceFindControllerModel(def, &disk->info, VIR_DOMAIN_CONTROLLER_TYPE_SCSI);

And this.

if ((qemuSetScsiControllerModel(def, qemuCaps, &controllerModel)) < 0) goto error; @@ -4370,6 +4379,86 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev) return ret; }

+char * +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *sg = NULL; + + if (!(sg = virSCSIDeviceGetDevStr(dev->source.subsys.u.scsi.adapter, + dev->source.subsys.u.scsi.bus, + dev->source.subsys.u.scsi.target, + dev->source.subsys.u.scsi.unit))) {

Indentions?

+ goto error; + } + + virBufferAsprintf(&buf, "file=/dev/%s,if=none", sg); + virBufferAsprintf(&buf, ",id=%s-%s", + virDomainDeviceAddressTypeToString(dev->info->type), + dev->info->alias); + if (dev->readonly && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) + virBufferAsprintf(&buf, ",readonly=on"); + if (virBufferError(&buf)) { + virReportOOMError(); + goto error; + } + VIR_FREE(sg);

Like what I commented in 5/10 for the frees. This can be simplified.

+ + return virBufferContentAndReset(&buf); + +error: + VIR_FREE(sg); + virBufferFreeAndReset(&buf); + return NULL; +} + + +char * +qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int controllerModel = -1; + + controllerModel = virDomainDeviceFindControllerModel(def, dev->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (qemuSetScsiControllerModel(def, qemuCaps, &controllerModel) < 0) + goto error; + /* TODO: deal with lsi or ibm controller */ + if (controllerModel == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) { + virBufferAsprintf(&buf, "scsi-generic"); + if (dev->info->addr.drive.bus != 0) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("SCSI controller only supports 1 bus")); + goto error; + } + /* TODO: deal with early version qemu which does not support bus... */ + virBufferAsprintf(&buf, + ",bus=scsi%d.0,channel=0,scsi-id=%d,lun=%d", + dev->info->addr.drive.controller, + dev->info->addr.drive.target, + dev->info->addr.drive.unit); + virBufferAsprintf(&buf, ",drive=%s-%s,id=%s", + virDomainDeviceAddressTypeToString(dev->info->type), + dev->info->alias, dev->info->alias); + if (dev->info->bootIndex) + virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unknown controller"));

Improper error: _("Unsupport controller model '%s'"), virDomainControllerModelSCSITypeToString(controllerModel))

+ goto error; + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto error; + } + + return virBufferContentAndReset(&buf); +error: + virBufferFreeAndReset(&buf); + return NULL; +}

/* This function outputs a -chardev command line option which describes only the @@ -7433,10 +7522,11 @@ qemuBuildCommandLine(virConnectPtr conn, if (hostdev->info->bootIndex) { if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)) { + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB && + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("booting from assigned devices is only" - " supported for PCI and USB devices")); + " supported for PCI, USB and SCSI devices")); goto error; } else { if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && @@ -7453,6 +7543,39 @@ qemuBuildCommandLine(virConnectPtr conn, " supported with this version of qemu")); goto error; } + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_GENERIC_BOOTINDEX)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("booting from assigned SCSI devices is not" + " supported with this version of qemu")); + goto error; + } + } + } + + /* SCSI */ + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_GENERIC)) { + char *drvstr; + + virCommandAddArg(cmd, "-drive"); + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) + goto error; + virCommandAddArg(cmd, drvstr); + VIR_FREE(drvstr); + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev, qemuCaps))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SCSI passthrough is not supported by this version of qemu")); + goto error; } }

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 17687f4..188f899 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -139,6 +139,12 @@ char * qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev); char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps);

+char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps); +char * qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps); + char * qemuBuildHubDevStr(virDomainHubDefPtr dev, virQEMUCapsPtr qemuCaps); char * qemuBuildRedirdevDevStr(virDomainDefPtr def, virDomainRedirdevDefPtr dev,

On 01/04/13 20:00, Han Cheng wrote:

Although virtio-scsi support SCSI PR, the device in host may do not support this. To avoid losing data, we only allow one scsi hostdev be passthrough to one guest.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 3 + src/qemu/qemu_hostdev.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hostdev.h | 10 ++ src/qemu/qemu_process.c | 3 + 5 files changed, 245 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c5ddaad..28d9685 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -37,6 +37,7 @@ # include "vircgroup.h" # include "virpci.h" # include "virusb.h" +# include "virscsi.h" # include "cpu_conf.h" # include "driver.h" # include "virportallocator.h" @@ -206,6 +207,7 @@ struct _virQEMUDriver { virPCIDeviceListPtr activePciHostdevs; virPCIDeviceListPtr inactivePciHostdevs; virUSBDeviceListPtr activeUsbHostdevs; + virSCSIDeviceListPtr activeScsiHostdevs;

We have to have a share module to manage these list, Chunyan Liu is doing it. But it doesn't affect this patch, unless there is conflicts.

/* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr sharedDisks; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 96bf235..408a2cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -682,6 +682,9 @@ qemuStartup(bool privileged, if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL) goto error;

+ if ((qemu_driver->activeScsiHostdevs = virSCSIDeviceListNew()) == NULL) + goto error; + if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDiskEntryFree))) goto error;

diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index bac38b5..96b3e8f 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "virpci.h" #include "virusb.h" +#include "virscsi.h" #include "virnetdev.h"

#define VIR_FROM_THIS VIR_FROM_QEMU @@ -214,6 +215,59 @@ cleanup: return ret; }

+int +qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + virDomainHostdevDefPtr hostdev = NULL; + int i; + int ret = -1; + + if (!def->nhostdevs) + return 0; + + virObjectLock(driver->activeScsiHostdevs); + for (i = 0; i < def->nhostdevs; i++) { + virSCSIDevicePtr scsi = NULL; + hostdev = def->hostdevs[i]; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue;

Can be more compact: if (hostdev->model != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) continue;

+ + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) { + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + def->name); + continue; + } + + virSCSIDeviceSetUsedBy(scsi, def->name); + + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) { + virSCSIDeviceFree(scsi); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to add SCSI device %d:%d:%d:%d to activeScsiHostdevs"), + virSCSIDeviceGetAdapter(scsi), virSCSIDeviceGetBus(scsi), + virSCSIDeviceGetTarget(scsi), virSCSIDeviceGetUnit(scsi)); + goto cleanup; + } + } + ret = 0; + +cleanup: + virObjectUnlock(driver->activeScsiHostdevs); + return ret; +} + static int qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) { @@ -811,6 +865,103 @@ cleanup: return ret; }

+int qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i, j, count; + virSCSIDeviceListPtr list; + virSCSIDevicePtr tmp; + + /* To prevent situation where SCSI device is assigned to two domains + * we need to keep a list of currently assigned SCSI devices. + * This is done in several loops which cannot be joined into one big + * loop. See qemuPrepareHostdevPCIDevices() + */ + if (!(list = virSCSIDeviceListNew())) + goto cleanup; + + /* Loop 1: build temporary list + */

/* Loop 1: build temporary list */

+ for (i = 0 ; i < nhostdevs ; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virSCSIDevicePtr scsi; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) + goto cleanup; + + if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } + } + + /* Loop 2: Mark devices in temporary list as used by @name + * and add them do driver list. However, if something goes

s/do/to/

+ * wrong, perform rollback. + */ + virObjectLock(driver->activeScsiHostdevs); + count = virSCSIDeviceListCount(list); + + for (i = 0; i < count; i++) { + virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i); + if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { + const char *other_name = virSCSIDeviceGetUsedBy(tmp); + + if (other_name) + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI device %s is in use by domain %s"), + virSCSIDeviceGetName(tmp), other_name); + else + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI device %s is already in use"), + virSCSIDeviceGetName(tmp)); + goto error; + } + + virSCSIDeviceSetUsedBy(scsi, name); + VIR_DEBUG("Adding %d:%d:%d:%d dom=%s to activeScsiHostdevs", + virSCSIDeviceGetAdapter(scsi), virSCSIDeviceGetBus(scsi), + virSCSIDeviceGetTarget(scsi), virSCSIDeviceGetUnit(scsi), name); + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) + goto error; + } + + virObjectUnlock(driver->activeScsiHostdevs); + + /* Loop 3: Temporary list was successfully merged with + * driver list, so steal all items to avoid freeing them + * in cleanup label. + */ + while (virSCSIDeviceListCount(list) > 0) { + tmp = virSCSIDeviceListGet(list, 0); + virSCSIDeviceListSteal(list, tmp); + } + + virObjectUnref(list); + return 0; + +error: + for (j = 0; j < i; j++) { + tmp = virSCSIDeviceListGet(list, i); + virSCSIDeviceListSteal(driver->activeScsiHostdevs, tmp); + } + virObjectUnlock(driver->activeScsiHostdevs); +cleanup: + virObjectUnref(list); + return -1; +} + int qemuPrepareHostDevices(virQEMUDriverPtr driver, virDomainDefPtr def, bool coldBoot) @@ -825,6 +976,10 @@ int qemuPrepareHostDevices(virQEMUDriverPtr driver, if (qemuPrepareHostUSBDevices(driver, def, coldBoot) < 0) return -1;

+ if (qemuPrepareHostdevSCSIDevices(driver, def->name, + def->hostdevs, def->nhostdevs) < 0) + return -1; + return 0; }

@@ -1009,6 +1164,75 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver, virObjectUnlock(driver->activeUsbHostdevs); }

+void +qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i; + + virObjectLock(driver->activeScsiHostdevs); + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virSCSIDevicePtr scsi, tmp; + const char *used_by = NULL; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + if (hostdev->missing) + continue;

These "continues" can be more compact.

+ + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) { + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + name); + continue; + } + + /* Delete only those SCSI devices which belongs + * to domain @name because qemuProcessStart() might + * have failed because SCSI device is already taken.

s/because/for/

+ * Therefore we want to steal only those devices from

s/steal/delete/

+ * the list which were taken by @name */

s/taken/used/ How about: /* Only delete the devices which are marked as being used by @name, * because qemuProcessStart could fail on the half way. */ How about one specify (managed='no|yes') for scsi hostdev? Osier

On 04/03/2013 06:20 PM, Osier Yang wrote:

On 01/04/13 20:01, Han Cheng wrote:

...

And user should hotplug a virtio-scsi controller if doesn't exist.

I'm wondering if it could be implicitly added.

As I know, adding controller implicitly is for back compatibility. New codes don't need to do this. Am I right?

...

Usb hostdev related codes are in qemuDomainAttachHostDevice, push down to qemuDomainAttachHostUsbDevice.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- src/qemu/qemu_hotplug.c | 211 ++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 173 insertions(+), 38 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b978b97..a78b410 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c + + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) + goto error; + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) + goto error; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) + goto error; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) {

vm->def->nhostdevs + 1

...

+ virReportOOMError(); + goto error; + }

vm->def->hostdevs[def->nhostdevs] is leaked if it errors out later.

I don't think so. vm->def->hostdevs[def->nhostdevs] is still tracked by vm->def->hostdevs. It is comsumed but without any data. It will be free when we free or realloc vm->def->hostdevs. And it is not big(sizeof(void *)). Besides, others code in this file deal with it in the same way. Actually, I find memory leak with usb code in the file. I fix it in this patch. I'll point it as following. Could you check it for me?

...

@@ -1123,6 +1215,26 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, int ret; qemuDomainObjPrivatePtr priv = vm->privateData; char *devstr = NULL; + virUSBDeviceList *list; + virUSBDevicePtr usb = NULL; + + + if (!(list = virUSBDeviceListNew())) + goto error; + + if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) + goto error; + + if (virUSBDeviceListAdd(list, usb) < 0) { + virUSBDeviceFree(usb); + usb = NULL; + goto error; + } + + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) { + usb = NULL; + goto error; + } if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) @@ -1138,7 +1250,6 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { virCgroupPtr cgroup = NULL; - virUSBDevicePtr usb; qemuCgroupData data; if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { @@ -1148,19 +1259,12 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, goto error; } - if ((usb = virUSBDeviceNew(hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device, - NULL)) == NULL) - goto error; - data.vm = vm; data.cgroup = cgroup; if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, &data) < 0) { - virUSBDeviceFree(usb); goto error; } - virUSBDeviceFree(usb); } qemuDomainObjEnterMonitor(driver, vm); @@ -1177,11 +1281,16 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + virUSBDeviceListSteal(list, usb); + virObjectUnref(list); VIR_FREE(devstr); return 0; error: + if (usb) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); + virObjectUnref(list); VIR_FREE(devstr); return -1; } @@ -1190,9 +1299,6 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - virUSBDeviceList *list; - virUSBDevicePtr usb = NULL; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev mode '%s' not supported"), @@ -1200,30 +1306,10 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, return -1; } - if (!(list = virUSBDeviceListNew())) - goto cleanup; - - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) - goto cleanup; - - if (virUSBDeviceListAdd(list, usb) < 0) { - virUSBDeviceFree(usb); - usb = NULL; - goto cleanup; - } - - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) { - usb = NULL; - goto cleanup; - } - - virUSBDeviceListSteal(list, usb);

We steal usb from list. *usb is tracked both by usb and driver->activeUsbHostdevs. If it errors later, we goto error: [1]

...

- } if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) - goto cleanup; + return -1; switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1238,6 +1324,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (qemuDomainAttachHostScsiDevice(driver, vm, + hostdev) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -1245,18 +1337,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; } - virObjectUnref(list); return 0; error: if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); - -cleanup: - virObjectUnref(list); - if (usb) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); [1] As *usb is not tracked by list, freeing list doesn't free *usb. We just steal not delete from driver->activeUsbHostdevs and don't free usb, so *usb leaks.

I fixed this by making stealing usb from list just before we return 0.

...

return -1; }

On 01/04/13 20:01, Han Cheng wrote:

This patch add tests for scsi hostdev.

s/add/adds/

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- tests/qemuhelpdata/qemu-1.0-device | 10 ++++++ tests/qemuhelpdata/qemu-1.1.0-device | 10 ++++++ tests/qemuhelpdata/qemu-1.2.0-device | 5 +++ tests/qemuhelpdata/qemu-kvm-1.2.0-device | 5 +++ tests/qemuhelptest.c | 19 ++++++++--- .../qemuxml2argv-hostdev-scsi-boot.args | 10 ++++++ .../qemuxml2argv-hostdev-scsi-boot.xml | 34 +++++++++++++++++++ .../qemuxml2argv-hostdev-scsi-readonly.args | 10 ++++++ .../qemuxml2argv-hostdev-scsi-readonly.xml | 35 ++++++++++++++++++++ .../qemuxml2argv-hostdev-scsi.args | 10 ++++++ .../qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml | 34 +++++++++++++++++++ tests/qemuxml2argvtest.c | 12 +++++++ tests/qemuxml2xmltest.c | 4 ++ 13 files changed, 193 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml

diff --git a/tests/qemuhelpdata/qemu-1.0-device b/tests/qemuhelpdata/qemu-1.0-device index 0bdfbbd..d557f0e 100644 --- a/tests/qemuhelpdata/qemu-1.0-device +++ b/tests/qemuhelpdata/qemu-1.0-device @@ -136,3 +136,13 @@ virtio-net-pci.romfile=string virtio-net-pci.rombar=uint32 virtio-net-pci.multifunction=on/off virtio-net-pci.command_serr_enable=on/off +scsi-generic.drive=drive +scsi-generic.logical_block_size=uint16 +scsi-generic.physical_block_size=uint16 +scsi-generic.min_io_size=uint16 +scsi-generic.opt_io_size=uint32 +scsi-generic.bootindex=int32

I might be wrong, but what I got is "bootindex" showed up since qemu 1.2.

+scsi-generic.discard_granularity=uint32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelpdata/qemu-1.1.0-device b/tests/qemuhelpdata/qemu-1.1.0-device index abbf850..7313a34 100644 --- a/tests/qemuhelpdata/qemu-1.1.0-device +++ b/tests/qemuhelpdata/qemu-1.1.0-device @@ -158,3 +158,13 @@ scsi-disk.dpofua=on/off scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32 +scsi-generic.drive=drive +scsi-generic.logical_block_size=blocksize +scsi-generic.physical_block_size=blocksize +scsi-generic.min_io_size=uint16 +scsi-generic.opt_io_size=uint32 +scsi-generic.bootindex=int32 +scsi-generic.discard_granularity=uint32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelpdata/qemu-1.2.0-device b/tests/qemuhelpdata/qemu-1.2.0-device index 5613e00..40845e4 100644 --- a/tests/qemuhelpdata/qemu-1.2.0-device +++ b/tests/qemuhelpdata/qemu-1.2.0-device @@ -208,3 +208,8 @@ usb-host.bootindex=int32 usb-host.pipeline=on/off usb-host.port=string usb-host.full-path=on/off +scsi-generic.drive=drive +scsi-generic.bootindex=int32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelpdata/qemu-kvm-1.2.0-device b/tests/qemuhelpdata/qemu-kvm-1.2.0-device index 879a049..09e3ef7 100644 --- a/tests/qemuhelpdata/qemu-kvm-1.2.0-device +++ b/tests/qemuhelpdata/qemu-kvm-1.2.0-device @@ -220,3 +220,8 @@ usb-host.bootindex=int32 usb-host.pipeline=on/off usb-host.port=string usb-host.full-path=on/off +scsi-generic.drive=drive +scsi-generic.bootindex=int32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index a28109a..156be16 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -506,7 +506,8 @@ mymain(void) QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_DEVICE_VMWARE_SVGA, QEMU_CAPS_DEVICE_USB_SERIAL, - QEMU_CAPS_DEVICE_USB_NET); + QEMU_CAPS_DEVICE_USB_NET, + QEMU_CAPS_SCSI_GENERIC); DO_TEST("qemu-kvm-0.12.1.2-rhel61", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -723,7 +724,9 @@ mymain(void) QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_DEVICE_VMWARE_SVGA, QEMU_CAPS_DEVICE_USB_SERIAL, - QEMU_CAPS_DEVICE_USB_NET); + QEMU_CAPS_DEVICE_USB_NET, + QEMU_CAPS_SCSI_GENERIC, + QEMU_CAPS_SCSI_GENERIC_BOOTINDEX); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -812,7 +815,9 @@ mymain(void) QEMU_CAPS_DEVICE_VMWARE_SVGA, QEMU_CAPS_DEVICE_USB_SERIAL, QEMU_CAPS_DEVICE_USB_NET, - QEMU_CAPS_DTB); + QEMU_CAPS_DTB, + QEMU_CAPS_SCSI_GENERIC, + QEMU_CAPS_SCSI_GENERIC_BOOTINDEX); DO_TEST("qemu-1.2.0", 1002000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -913,7 +918,9 @@ mymain(void) QEMU_CAPS_DEVICE_USB_SERIAL, QEMU_CAPS_DEVICE_USB_NET, QEMU_CAPS_DTB, - QEMU_CAPS_SCSI_MEGASAS); + QEMU_CAPS_SCSI_MEGASAS, + QEMU_CAPS_SCSI_GENERIC, + QEMU_CAPS_SCSI_GENERIC_BOOTINDEX); DO_TEST("qemu-kvm-1.2.0", 1002000, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -1019,7 +1026,9 @@ mymain(void) QEMU_CAPS_DEVICE_USB_SERIAL, QEMU_CAPS_DEVICE_USB_NET, QEMU_CAPS_DTB, - QEMU_CAPS_SCSI_MEGASAS); + QEMU_CAPS_SCSI_MEGASAS, + QEMU_CAPS_SCSI_GENERIC, + QEMU_CAPS_SCSI_GENERIC_BOOTINDEX);

As said in 4/10, this should be merged into 4/10.

return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.args new file mode 100644 index 0000000..470bd78 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -device \ +virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb -drive \ +file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-0 -device \ +ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive \ +file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0 -device \ +scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ +drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0,bootindex=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.xml new file mode 100644 index 0000000..e3de719 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-boot.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <boot order='1'/> + <address type='drive' controller='0' bus='0' target='4' unit='8'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args new file mode 100644 index 0000000..da74789 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc \ +-m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device \ +virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb -drive \ +file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-0 -device \ +ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive \ +file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0,readonly=on -device \ +scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ +drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml new file mode 100644 index 0000000..11d1712 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-readonly.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <readonly/> + <address type='drive' controller='0' bus='0' target='4' unit='8'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.args new file mode 100644 index 0000000..4c09359 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device \ +virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 -usb -drive \ +file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-0 -device \ +ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -drive \ +file=/dev/sg0,if=none,id=drive-hostdev-scsi_host0-0-0-0 -device \ +scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\ +drive=drive-hostdev-scsi_host0-0-0-0,id=hostdev-scsi_host0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml new file mode 100644 index 0000000..ccdbb0d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml

No need for this, as qemuxml2argv-hostdev-scsi-boot and qemuxml2argv-hostdev-scsi-readonly already cover the required testing.

@@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <address type='drive' controller='0' bus='0' target='4' unit='8'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 38787ac..64a7540 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -929,6 +929,18 @@ mymain(void)

DO_TEST("ppc-dtb", QEMU_CAPS_KVM, QEMU_CAPS_DTB);

+ DO_TEST("hostdev-scsi", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_GENERIC); + DO_TEST("hostdev-scsi-boot", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_GENERIC, + QEMU_CAPS_BOOTINDEX, QEMU_CAPS_SCSI_GENERIC_BOOTINDEX); + DO_TEST("hostdev-scsi-readonly", + QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_READONLY, + QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_GENERIC); + virObjectUnref(driver.config); virObjectUnref(driver.caps); virObjectUnref(driver.xmlconf); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ba9aa96..106ebe8 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -272,6 +272,10 @@ mymain(void)

DO_TEST_DIFFERENT("metadata");

+ DO_TEST("hostdev-scsi"); + DO_TEST("hostdev-scsi-boot"); + DO_TEST("hostdev-scsi-readonly"); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlconf);