Re: [libvirt] [PATCH 1/4] virsh-domain: use correct base for virStrToLong_ui

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(a)redhat.com&gt; > > --- > > tools/virsh-domain.c | 30 +++++++++++++++--------------- > > 1 file changed, 15 insertions(+), 15 deletions(-) > > Peter

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list