[libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui
phrdina at redhat.com
Fri Oct 23 07:59:02 UTC 2015
On Fri, Oct 23, 2015 at 09:33:49AM +0200, Peter Krempa wrote:
> On Thu, Oct 22, 2015 at 11:24:02 -0400, Laine Stump wrote:
> > On 10/21/2015 08:22 AM, Pavel Hrdina wrote:
> > > While parsing device addresses we should use correct base and don't
> > > count on auto-detect. For example, PCI address uses hex numbers, but
> > > each number starting with 0 will be auto-detected as octal number and
> > > that's wrong. Another wrong use-case is for PCI address if for example
> > > bus is 10, than it's incorrectly parsed as decimal number.
> > >
> > > PCI and CCW addresses have all values as hex numbers, IDE and SCSI
> > > addresses are in decimal numbers.
> > I've seen examples for PCI that use decimal a number for the slot (which
> > is the one item that's likely to have a value > 7 unless you have a
> > system with a whole bunch of PCI controllers)[*], and those that use hex
> > numbers always prefix the number with "0x". libvirt itself always a
> lspci doesn't really use 0x prefix:
> 00:16.0 Communication controller: Intel Corporation Wildcat Point-LP MEI Controller #1 (rev 03)
> 00:19.0 Ethernet controller: Intel Corporation Ethernet Connection (3) I218-LM (rev 03)
> 00:1b.0 Audio device: Intel Corporation Wildcat Point-LP High Definition Audio Controller (rev 03)
> 00:1c.0 PCI bridge: Intel Corporation Wildcat Point-LP PCI Express Root Port #6 (rev e3)
> Btw that's a laptop, not a super special system.
> > prefixes the domain, bus, and slot with 0x, so an auto-detected base
> > will always get those right.
> Well, not entirely:
> nodedev identificators use hex, possibly padded with zeroes:
> Which may result also in some octal 'fun' on boxes with 16 buses ;).
> nodedev XML uses decimal in the parsed address:
> $ virsh nodedev-dumpxml pci_0000_00_19_0
> <capability type='pci'>
> <product id='0x15a2'>Ethernet Connection (3) I218-LM</product>
> <vendor id='0x8086'>Intel Corporation</vendor>
> (Uhhh, so 19 ... or 25? ... or perhaps 31?)
> And for hostdevs we are promoting the use of 0x prefixed hex:
> <address domain='0x0000' bus='0x06' slot='0x02' function='0x0'/>
> As well as with target addresses. Fortunately.
> <sarcasm>Well that's very consistent.</sarcasm>
> > So I I think the existing code is correct, and don't see an upside to
> > making this restriction/invisible change in semantics, and it could
> > potentially lead to incorrect results if someone is thinking that
> > they're entering decimal numbers.
> The existing code is correct only perhaps from a historical point of
> view. From a functional it's more than flawed. ...
> > [*] There was one particular document that even went to the trouble of
> > explaining how to convert the hex value of slot into a decimal number to
> > use in the libvirt config!
> Well, this hints that we do actually something wrong here. The ambiguity
> in parsing of numbers with virStrToLong_ui has already bitten us (I
> won't bother looking up the commit though).
> The problem is that if you use 0 as a base argument it's not really
> clear what you've parsed and that will result in situations like this.
> The developer may be lucky in trying numbers that were parsed correctly
> misleading him that he used the correct base.
> I think that we shouldn't be using 0 as base in ANY place in our code.
> As of this particular case: If we format it in base 16, we should parse
> it in base 16. Having ambiguity in the parser code will only end up in
> > >
> > > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > > ---
> > > tools/virsh-domain.c | 30 +++++++++++++++---------------
> > > 1 file changed, 15 insertions(+), 15 deletions(-)
> > >
A agree with Peter, as there is no reasonable point to represent PCI address in
decimal numbers, only libvirt does that and as said, it's wrong. I don't know
any other place, where the PCI address is printed as decimal number.
I've been motivated to update this behavior after I've used '0000:05:10.1' as
an argument for the '--srouce' and it parsed the slot as decimal number and
printed in to the XML as '0x0a'.
> libvir-list mailing list
> libvir-list at redhat.com
More information about the libvir-list