On 06/15/2015 05:03 PM, John Ferlan wrote:
Something I didn't catch on pass1, but perhaps an important distinction
title:
Resolve issues with SCSI LUN hostdev device addressing
Sounds good! I'll work that into the split patches.
Coincidentally - there's a bz regarding the setting between
SCSI and which is assigned to me and was near the top
of my todo list... https://bugzilla.redhat.com/show_bug.cgi?id=1210587
so this has helped put me in the right frame of reference!
...
Need to add some XML in order to "test" the longer values - eg, xml2xml
and/or xml2args in order to validate that what you read in is what gets
printed out and that what you read in gets sent to the hypervisor
correctly.
I wrestled with this part, because the xml2args tests pass the SCSI
[host:bus:target:unit] address to testSCSIDeviceGetSgName, which ignores
all inputs and returns /dev/sg0. The reverse had a similar gotcha.
I'll dig at xml2xml, and see what I can come up with.
wonders what qemu dos with a 20 digit unit number...
So speaking of qemu... If you look at qemuBuildDriveStr you will see how
the device address is passed down to qemu... That function needs some
adjustment too it seems.
This function doesn't appear to be invoked for an tag within a
element, either as part of the subelements or as the
address being created for the guest. Rather, it seems to turn up as
part of a element, which (unless I'm mistaken) relies on the host
"/dev" address instead of [host:bus:target:unit].
Ah... so back to my title comment ;-) - hostdev... That would mean
qemuBuildSCSIHostdevDevStr, which does have a printing of bus, target,
unit. Still wondering about how qemu would handle it (haven't got that
far yet), but that seems to be what you point out in the next paragraph.
Speaking of the address that is created in the guest, though, I left
that as-is because virtio defines LUN addresses up to 16,384 and is
enforced in KVM/QEMU. Since I can't create a unit address in the guest
larger than that, leaving that defined as an int is okay.
And so regardless of what you did for libvirt, kvm/qemu wouldn't be able
to handle it? Perhaps then that needs to be checked somehow... Is this
something qemu/kvm needs to support?
I'll defer to you, but kvm/qemu enforces things okay and libvirt fails
gracefully. Example:
# cat disk.xml
unit='16384'/>
# virsh attach-device lmb_guest disk.xml
error: Failed to attach device from disk.xml
error: internal error: unable to execute QEMU command 'device_add': bad
scsi device lun: 16384
# vim disk.xml
# cat disk.xml
unit='16383'/>
# virsh attach-device lmb_guest disk.xml
Device attached successfully
This looks reasonable to me because virtio only permits 256 targets, and
16,384 units per targets. So there's no concern with the corresponding
"int" fields being overrun.
Again I haven't looked at those
sources yet. Is there a specific hypervisor that isn't working because
libvirt isn't passing the correct address value? This is the "what is
the bug you're trying to fix" type question...
I'm using qemu/kvm, but I haven't seen anything besides the "virsh edit"
and "virt-xml-validate" errors when the hostdev->source->address->unit
value is larger than two digits. But this really boils to your later
comment about the patch being split, to explain the sequence of events
that brought me here. I have it broken into the patches you had
suggested, plus two others (one doc, one xml schema which is what
started this), but haven't tested them independently yet. Tomorrow, I hope.
Eric
I didn't dig into the qemu sources yet, but
Looks like I also lost my train of thought here ;-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 1781996..e7a8e1a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2827,7 +2827,7 @@
Drive addresses have the following additional
attributes: controller (a 2-digit controller
number), bus (a 2-digit bus number),
-target (a 2-digit bus number),
+target (a 2-digit target number),
and unit (a 2-digit unit number on the bus).
type='virtio-serial'
Interesting there's no 'scsi' in here (of course you're removing it
below, but that leaves the address as unknown
Is it? See below, but I thought that got hardcoded somewhere because
it's in a hostdev. I'll doublecheck.
It seems virDomainHostdevSubsysSCSIHostDefParseXML ignores the type
field and the RNG supports that.
@@ -3136,7 +3136,7 @@
-
+
These first two don't seem relevant to the changes below and should have
been their own patc
Something I didn't catch on pass1, but perhaps an important distinction
title:
Resolve issues with SCSI LUN hostdev device addressing
Coincidentally - there's a bz regarding the setting between
SCSI and which is assigned to me and was near the top
of my todo list... https://bugzilla.redhat.com/show_bug.cgi?id=1210587
so this has helped put me in the right frame of reference!
...
>> Need to add some XML in order to "test" the longer values - eg, xml2xml
>> and/or xml2args in order to validate that what you read in is what gets
>> printed out and that what you read in gets sent to the hypervisor
>> correctly.
>
> I wrestled with this part, because the xml2args tests pass the SCSI
> [host:bus:target:unit] address to testSCSIDeviceGetSgName, which ignores
> all inputs and returns /dev/sg0. The reverse had a similar gotcha.
> I'll dig at xml2xml, and see what I can come up with.
>
>> wonders what qemu dos with a 20 digit unit number...
>>
>> So speaking of qemu... If you look at qemuBuildDriveStr you will see how
>> the device address is passed down to qemu... That function needs some
>> adjustment too it seems.
>
> This function doesn't appear to be invoked for an tag within a
> element, either as part of the subelements or as the
> address being created for the guest. Rather, it seems to turn up as
> part of a element, which (unless I'm mistaken) relies on the host
> "/dev" address instead of [host:bus:target:unit].
Ah... so back to my title comment ;-) - hostdev... That would mean
qemuBuildSCSIHostdevDevStr, which does have a printing of bus, target,
unit. Still wondering about how qemu would handle it (haven't got that
far yet), but that seems to be what you point out in the next paragraph.
>
> Speaking of the address that is created in the guest, though, I left
> that as-is because virtio defines LUN addresses up to 16,384 and is
> enforced in KVM/QEMU. Since I can't create a unit address in the guest
> larger than that, leaving that defined as an int is okay.
>
And so regardless of what you did for libvirt, kvm/qemu wouldn't be able
to handle it? Perhaps then that needs to be checked somehow... Is this
something qemu/kvm needs to support? Again I haven't looked at those
sources yet. Is there a specific hypervisor that isn't working because
libvirt isn't passing the correct address value? This is the "what is
the bug you're trying to fix" type question...
>>
>> I didn't dig into the qemu sources yet, but
>>
Looks like I also lost my train of thought here ;-)
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index 1781996..e7a8e1a 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -2827,7 +2827,7 @@
>>> Drive addresses have the following additional
>>> attributes: controller (a 2-digit controller
>>> number), bus (a 2-digit bus number),
>>> -target (a 2-digit bus number),
>>> +target (a 2-digit target number),
>>> and unit (a 2-digit unit number on the bus).
>>>
>>> type='virtio-serial'
>> Interesting there's no 'scsi' in here (of course you're removing it
>> below, but that leaves the address as unknown
>
> Is it? See below, but I thought that got hardcoded somewhere because
> it's in a hostdev. I'll doublecheck.
>
It seems virDomainHostdevSubsysSCSIHostDefParseXML ignores the type
field and the RNG supports that.
>>
>>
>>> @@ -3136,7 +3136,7 @@
>>> >> rawio='yes'>
>>>
>>>
>>> -
>>> +
>> These first two don't seem relevant to the changes below and should have
>> been their own patch.
>
> Fair enough, my apologies. I'll break them out.
>
We all fall into the same trap at some point in time ;-)
>>
>> See [1] below for a side comment on this particular changes
>>
...
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 36de844..1f2bfca 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -4954,7 +4954,7 @@
>>> virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode,
>>> goto cleanup;
>>> }
>>> -if (virStrToLong_ui(unit, NULL, 0,
>>> &scsihostsrc->unit) < 0) {
>>> +if (virStrToLong_ull(unit, NULL, 0,
>>> &scsihostsrc->unit) < 0) {
>>> virReportError(VIR_ERR_INTERNAL_ERROR,
>>> _("cannot parse unit '%s'"), unit);
>> Existing - read as "ui" (or now "ull"), but could be "uip" and "ullp"
>> (eg, don't allow reading a negative value).
>>
>> I would be remiss if I didn't point out UINT_MAX is 4294967295U, which
>> is 10 digits, but means 99 which would be 'valid' according to
>> the 10 digit rule in RNG but invalid once
On 06/12/2015 04:49 PM, John Ferlan wrote:
On 06/10/2015 11:22 AM, Eric Farman wrote:
Defining a domain with a SCSI disk attached via a hostdev
tag and a source address unit value longer than two digits
causes an error when editing the domain with virsh edit,
even if no changes are made to the domain definition.
The error suggests invalid XML, somewhere:
# virsh edit lmb_guest
error: XML document failed to validate against schema:
Unable to validate doc against /usr/local/share/libvirt/schemas/domain.rng
Extra element devices in interleave
Element domain failed to validate content
The virt-xml-validate tool fails with a similar error:
# virt-xml-validate lmb_guest.xml
Relax-NG validity error : Extra element devices in interleave
lmb_guest.xml:17: element devices: Relax-NG validity error :
Element domain failed to validate content
lmb_guest.xml fails to validate
The hostdev tag requires a source address to be specified,
which includes bus, target, and unit address attributes.
According to the SCSI Architecture Model spec (section
4.9 of SAM-2), a LUN address is 64 bits and thus could be
up to 20 decimal digits long. Unfortunately, the XML
schema limits this string to just two digits. Similarly,
the target field can be up to 32 bits in length, which
would be 10 decimal digits.
# lsscsi -xx
[0:0:19:0x40224011] diskIBM 2107900 3.44 /dev/sda
# lsscsi
[0:0:19:1074872354]diskIBM 2107900 3.44 /dev/sda
# cat lmb_guest.xml
lmb_guest
1024
...trimmed...
...trimmed...
Since the reference unit and target fields are used in
several places in the XML schema, create a separate one
specific for SCSI Logical Units that will permit the
greater length. Also, expand the definition of the SCSI
unit field from an int to a long long, to cover the
possible size. This permits both the validation utility
and the virsh edit command to succeed when a hostdev
tag is included.
Nice explanation - helped set the stage quite well!
Of course you've just opened Pandora's box of questions...
Signed-off-by: Eric Farman
Reviewed-by: Matthew Rosato
Reviewed-by: Stefan Zimmermann
Reviewed-by: Boris Fiuczynski
---
docs/formatdomain.html.in | 10 +++---
docs/schemas/domaincommon.rng | 14 --
src/conf/domain_audit.c | 2 +-
src/conf/domain_conf.c| 4 ++--
src/conf/domain_conf.h| 2 +-
src/qemu/qemu_command.h | 2 +-
src/qemu/qemu_hotplug.c | 4 ++--
src/util/virhostdev.c | 6 +++---
src/util/virscsi.c| 16
src/util/virscsi.h| 8
tests/testutilsqemu.c | 2 +-
tools/virsh-domain.c | 6 +++---
12 files changed, 45 insertions(+), 31 deletions(-)
Need to add some XML in order to "test" the longer values - eg, xml2xml
and/or xml2args in order to validate that what you read in is what gets
printed out and that what you read in gets sent to the hypervisor
correctly.
I wrestled with this part, because the xml2args tests pass the SCSI
[host:bus:target:unit] address to testSCSIDeviceGetSgName, which ignores
all inputs and returns /dev/sg0. The reverse had a similar gotcha.
I'll dig at xml2xml, and see what I can come up with.
wonders what qemu dos with a 20 digit unit number...
So speaking of qemu... If you look at qemuBuildDriveStr you will see how
the device address is passed down to qemu... That function needs some
adjustment too it seems.
This function doesn't appear to be invoked for an tag within a
element, either as part of the subelements or as the
address being created for the guest. Rather, it seems to turn up as
part of a element, which (unless I'm mistaken) relies on the host
"/dev" address instead of [host:bus:target:unit].
Speaking of the address that is created in the guest, though, I left
that as-is because virtio defines LUN addresses up to 16,384 and is
enforced in KVM/QEMU. Since I can't create a unit address in the guest
larger than that, leaving that defined as an int is okay.
I didn't dig into the qemu sources yet, but
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 1781996..e7a8e1a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2827,7 +2827,7 @@
Drive addresses have the following additional
attributes: controller (a 2-digit controller
number), bus (a 2-digit bus number),
-target (a 2-digit bus number),
+target (a 2-digit target number),
and unit (a 2-digit unit number on the bus).
type='virtio-serial'
Interesting there's no 'scsi' in here (of course you're removing it
below, but that leaves the address as unknown
Is it? See below, but I thought that got hardcoded somewhere because
it's in a hostdev. I'll doublecheck.
On 06/10/2015 11:22 AM, Eric Farman wrote:
> Defining a domain with a SCSI disk attached via a hostdev
> tag and a source address unit value longer than two digits
> causes an error when editing the domain with virsh edit,
> even if no changes are made to the domain definition.
> The error suggests invalid XML, somewhere:
>
> # virsh edit lmb_guest
> error: XML document failed to validate against schema:
> Unable to validate doc against /usr/local/share/libvirt/schemas/domain.rng
> Extra element devices in interleave
> Element domain failed to validate content
>
> The virt-xml-validate tool fails with a similar error:
>
> # virt-xml-validate lmb_guest.xml
> Relax-NG validity error : Extra element devices in interleave
> lmb_guest.xml:17: element devices: Relax-NG validity error :
> Element domain failed to validate content
> lmb_guest.xml fails to validate
>
> The hostdev tag requires a source address to be specified,
> which includes bus, target, and unit address attributes.
> According to the SCSI Architecture Model spec (section
> 4.9 of SAM-2), a LUN address is 64 bits and thus could be
> up to 20 decimal digits long. Unfortunately, the XML
> schema limits this string to just two digits. Similarly,
> the target field can be up to 32 bits in length, which
> would be 10 decimal digits.
>
> # lsscsi -xx
> [0:0:19:0x40224011] diskIBM 2107900 3.44 /dev/sda
> # lsscsi
> [0:0:19:1074872354]diskIBM 2107900 3.44 /dev/sda
> # cat lmb_guest.xml
>
> lmb_guest
> 1024
> ...trimmed...
>
>
>
>
>
>
>
>
> ...trimmed...
>
> Since the reference unit and target fields are used in
> several places in the XML schema, create a separate one
> specific for SCSI Logical Units that will permit the
> greater length. Also, expand the definition of the SCSI
> unit field from an int to a long long, to cover the
> possible size. This permits both the validation utility
> and the virsh edit command to succeed when a hostdev
> tag is included.
>
Nice explanation - helped set the stage quite well!
Of course you've just opened Pandora's box of questions...
> Signed-off-by: Eric Farman
> Reviewed-by: Matthew Rosato
> Reviewed-by: Stefan Zimmermann
> Reviewed-by: Boris Fiuczynski
> ---
> docs/formatdomain.html.in | 10 +++---
> docs/schemas/domaincommon.rng | 14 --
> src/conf/domain_audit.c | 2 +-
> src/conf/domain_conf.c| 4 ++--
> src/conf/domain_conf.h| 2 +-
> src/qemu/qemu_command.h | 2 +-
> src/qemu/qemu_hotplug.c | 4 ++--
> src/util/virhostdev.c | 6 +++---
> src/util/virscsi.c| 16
> src/util/virscsi.h| 8
> tests/testutilsqemu.c | 2 +-
> tools/virsh-domain.c | 6 +++---
> 12 files changed, 45 insertions(+), 31 deletions(-)
>
Need to add some XML in order to "test" the longer values - eg, xml2xml
and/or xml2args in order to validate that what you read in is what gets
printed out and that what you read in gets sent to the hypervisor
correctly. wonders what qemu dos with a 20 digit unit number...
So speaking of qemu... If you look at qemuBuildDriveStr you will see how
the device address is passed down to qemu... That function needs some
adjustment too it seems.
I didn't dig into the qemu sources yet, but
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 1781996..e7a8e1a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2827,7 +2827,7 @@
>Drive addresses have the following additional
> attributes: controller (a 2-digit controller
> number), bus (a 2-digit bus number),
> -target (a 2-digit bus number),
> +target (a 2-digit target number),
> and unit (a 2-digit unit number on the bus).
>
>type='virtio-serial'
Interesting there's no 'scsi' in here (of course you're removing it
below, but that leaves the address as unknown
> @@ -3136,7 +3136,7 @@
>
>
>
> -
> +
These first two don't seem relevant to the changes below and should have
been their own patch.
See [1] below for a side comment on this particular changes
>
>
> unit='0'/>
> @@ -3244,7 +3244,11 @@
>
>scsi
>SCSI devices are described by both the adapter
> -and address elements.
> +and address elements. The address
> +element includes a bus attribute (a 2-digit bus
> +number), a target attribute (a 10-digit target
> +
Defining a domain with a SCSI disk attached via a hostdev
tag and a source address unit value longer than two digits
causes an error when editing the domain with virsh edit,
even if no changes are made to the domain definition.
The error suggests invalid XML, somewhere:
# virsh edit lmb_guest
error: XML document failed to validate against schema:
Unable to validate doc against /usr/local/share/libvirt/schemas/domain.rng
Extra element devices in interleave
Element domain failed to validate content
The virt-xml-validate tool fails with a similar error:
# virt-xml-validate lmb_guest.xml
Relax-NG validity error : Extra element devices in interleave
lmb_guest.xml:17: element devices: Relax-NG validity error :
Element domain failed to validate content
lmb_guest.xml fails to validate
The hostdev tag requires a source address to be specified,
which includes bus, target, and unit address attributes.
According to the SCSI Architecture Model spec (section
4.9 of SAM-2), a LUN address is 64 bits and thus could be
up to 20 decimal digits long. Unfortunately, the XML
schema limits this string to just two digits. Similarly,
the target field can be up to 32 bits in length, which
would be 10 decimal digits.
# lsscsi -xx
[0:0:19:0x40224011] diskIBM 2107900 3.44 /dev/sda
# lsscsi
[0:0:19:1074872354]diskIBM 2107900 3.44 /dev/sda
# cat lmb_guest.xml
lmb_guest
1024
...trimmed...
...trimmed...
Since the reference unit and target fields are used in
several places in the XML schema, create a separate one
specific for SCSI Logical Units that will permit the
greater length. Also, expand the definition of the SCSI
unit field from an int to a long long, to cover the
possible size. This permits both the validation utility
and the virsh edit command to succeed when a hostdev
tag is included.
Signed-off-by: Eric Farman
Reviewed-by: Matthew Rosato
Reviewed-by: Stefan Zimmermann
Reviewed-by: Boris Fiuczynski
---
docs/formatdomain.html.in | 10 +++---
docs/schemas/domaincommon.rng | 14 --
src/conf/domain_audit.c | 2 +-
src/conf/domain_conf.c| 4 ++--
src/conf/domain_conf.h| 2 +-
src/qemu/qemu_command.h | 2 +-
src/qemu/qemu_hotplug.c | 4 ++--
src/util/virhostdev.c | 6 +++---
src/util/virscsi.c| 16
src/util/virscsi.h| 8
tests/testutilsqemu.c | 2 +-
tools/virsh-domain.c | 6 +++---
12 files changed, 45 insertions(+), 31 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 1781996..e7a8e1a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2827,7 +2827,7 @@
Drive addresses have the following additional
attributes: controller (a 2-digit controller
number), bus (a 2-digit bus number),
-target (a 2-digit bus number),
+target (a 2-digit target number),
and unit (a 2-digit unit number on the bus).
type='virtio-serial'
@@ -3136,7 +3136,7 @@
-
+
@@ -3244,7 +3244,11 @@
scsi
SCSI devices are described by both the adapter
-and address elements.
+and address elements. The address
+element includes a bus attribute (a 2-digit bus
+number), a target attribute (a 10-digit target
+number), and a unit attribute (a 20-digit unit
+number on the bus).
Since 1.2.8, the source
element of a SCSI device may contain the protocol
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 5dc48f7..dccd3fd 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3846,10 +3846,10 @@
-
+
-
+
@@ -5142,11 +5142,21 @@
[0-9]{1,2}
+
+
+ [0-9]{1,10}
+
+
[0-9]{1,2}
+
+
+ [0-9]{1,20}
+
+
[a-zA-Z0-9\-_\.]+
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 1900039..844297c 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -427,7 +427,7 @@ virDomainAuditHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev,
} else {
virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
&scsisrc->u.host;
-if (virAsprintfQuiet(&address, "%s:%d:%d:%d",
+if (virAspri
