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>
> ---
> tools/virsh-domain.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
Peter
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)