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: > > pci_0000_00_03_0 > pci_0000_00_14_0 > pci_0000_00_16_0 > pci_0000_00_19_0 > pci_0000_00_1b_0 > > 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 > <device> > <name>pci_0000_00_19_0</name> > <path>/sys/devices/pci0000:00/0000:00:19.0</path> > <parent>computer</parent> > <driver> > <name>e1000e</name> > </driver> > <capability type='pci'> > <domain>0</domain> > <bus>0</bus> > <slot>25</slot> > <function>0</function> > <product id='0x15a2'>Ethernet Connection (3) I218-LM</product> > <vendor id='0x8086'>Intel Corporation</vendor> > </capability> > </device> > > (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 > problems. > > > > > > > > > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx> > > > --- > > > tools/virsh-domain.c | 30 +++++++++++++++--------------- > > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > > Peter 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'. Pavel > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list