Re: [libvirt] [PATCH] Fix virsh edit and virt-xml-validate with SCSI LUN

> 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. > <me> 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 <address> tag within a <hostdev> element, either as part of the <source> subelements or as the address being created for the guest. Rather, it seems to turn up as part of a <disk> 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 @@ >> <dd>Drive addresses have the following additional >> attributes: <code>controller</code> (a 2-digit controller >> number), <code>bus</code> (a 2-digit bus number), >> - <code>target</code> (a 2-digit bus number), >> + <code>target</code> (a 2-digit target number), >> and <code>unit</code> (a 2-digit unit number on the bus). >> </dd> >> <dt><code>type='virtio-serial'</code></dt> > 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.

> > >> @@ -3136,7 +3136,7 @@ >> &lt;hostdev mode='subsystem' type='scsi' sgio='filtered' >> rawio='yes'&gt; >> &lt;source&gt; >> &lt;adapter name='scsi_host0'/&gt; >> - &lt;address type='scsi' bus='0' target='0' unit='0'/&gt; >> + &lt;address bus='0' target='0' unit='0'/&gt; > 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.

> > 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 9999999999 which would be 'valid' according to > the 10 digit rule in RNG but invalid once you read it in... > >> goto cleanup; >> @@ -18844,7 +18844,7 @@ virDomainHostdevDefFormatSubsys(virBufferPtr >> buf, >> virBufferAsprintf(buf, "<adapter name='%s'/>\n", >> scsihostsrc->adapter); >> virBufferAsprintf(buf, >> - "<address %sbus='%d' target='%d' >> unit='%d'/>\n", >> + "<address %sbus='%d' target='%d' >> unit='%lld'/>\n", >> includeTypeInAddr ? "type='scsi' " : "", > Printed as "%d" not "%u" (of course that only means something for -1... > Without even considering the math, my eyes saw the commit message > "unit='1074872354'" and I immediately thought - is this one of those > negative value type issues... > > BTW: I wonder if this deserves a patch prior to this to use _uip and > avoid negative values and a second patch to change the formatting of %d > to %u. I like this idea. Will work through it. > > [1] Doesn't this provide an example of the 2nd of the two > formatdomain.html.in changes you made? That is you removed having > 'scsi' in the "<address...", but this shows that it is printed.... I see three places that call virDomainHostdevDefFormatSubsys, but the one where a <hostdev mode='subsystem'> is handled makes the call with includeTypeInAddr set to false. The other two callers handle network definitions and make the call with it set to true, but those won't drop into this section of code. This is why I pulled the type='scsi' from the doc above, because the existing code doesn't allow a type='scsi' to be specified. The routine virDomainHostdevSubsysSCSIHostDefParseXML only parses bus, target, and unit parameters in its address tag.

> > It seems you've covered most of the various areas. It may be easier to > review them if they were smaller units. Just making sure that "units" here is in reference to breaking up this patch for the ui->uip and %d->%u changes, and not the "logical unit" in the commit message?