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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]