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

[Laine Stump <laine@xxxxxxxxx>]

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 
prefixes the domain, bus, and slot with 0x, so an auto-detected base 
will always get those right.

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.

[*] 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!

> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
>   tools/virsh-domain.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 4191548..e8503ec 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -455,19 +455,19 @@ static int str2PCIAddress(const char *str, struct PCIAddress *pciAddr)
>   
>       domain = (char *)str;
>   
> -    if (virStrToLong_ui(domain, &bus, 0, &pciAddr->domain) != 0)
> +    if (virStrToLong_ui(domain, &bus, 16, &pciAddr->domain) != 0)
>           return -1;
>   
>       bus++;
> -    if (virStrToLong_ui(bus, &slot, 0, &pciAddr->bus) != 0)
> +    if (virStrToLong_ui(bus, &slot, 16, &pciAddr->bus) != 0)
>           return -1;
>   
>       slot++;
> -    if (virStrToLong_ui(slot, &function, 0, &pciAddr->slot) != 0)
> +    if (virStrToLong_ui(slot, &function, 16, &pciAddr->slot) != 0)
>           return -1;
>   
>       function++;
> -    if (virStrToLong_ui(function, NULL, 0, &pciAddr->function) != 0)
> +    if (virStrToLong_ui(function, NULL, 16, &pciAddr->function) != 0)
>           return -1;
>   
>       return 0;
> @@ -484,15 +484,15 @@ static int str2SCSIAddress(const char *str, struct SCSIAddress *scsiAddr)
>   
>       controller = (char *)str;
>   
> -    if (virStrToLong_uip(controller, &bus, 0, &scsiAddr->controller) != 0)
> +    if (virStrToLong_uip(controller, &bus, 10, &scsiAddr->controller) != 0)
>           return -1;
>   
>       bus++;
> -    if (virStrToLong_uip(bus, &unit, 0, &scsiAddr->bus) != 0)
> +    if (virStrToLong_uip(bus, &unit, 10, &scsiAddr->bus) != 0)
>           return -1;
>   
>       unit++;
> -    if (virStrToLong_ullp(unit, NULL, 0, &scsiAddr->unit) != 0)
> +    if (virStrToLong_ullp(unit, NULL, 10, &scsiAddr->unit) != 0)
>           return -1;
>   
>       return 0;
> @@ -509,15 +509,15 @@ static int str2IDEAddress(const char *str, struct IDEAddress *ideAddr)
>   
>       controller = (char *)str;
>   
> -    if (virStrToLong_ui(controller, &bus, 0, &ideAddr->controller) != 0)
> +    if (virStrToLong_ui(controller, &bus, 10, &ideAddr->controller) != 0)
>           return -1;
>   
>       bus++;
> -    if (virStrToLong_ui(bus, &unit, 0, &ideAddr->bus) != 0)
> +    if (virStrToLong_ui(bus, &unit, 10, &ideAddr->bus) != 0)
>           return -1;
>   
>       unit++;
> -    if (virStrToLong_ui(unit, NULL, 0, &ideAddr->unit) != 0)
> +    if (virStrToLong_ui(unit, NULL, 10, &ideAddr->unit) != 0)
>           return -1;
>   
>       return 0;
> @@ -534,15 +534,15 @@ static int str2CCWAddress(const char *str, struct CCWAddress *ccwAddr)
>   
>       cssid = (char *)str;
>   
> -    if (virStrToLong_ui(cssid, &ssid, 0, &ccwAddr->cssid) != 0)
> +    if (virStrToLong_ui(cssid, &ssid, 16, &ccwAddr->cssid) != 0)
>           return -1;
>   
>       ssid++;
> -    if (virStrToLong_ui(ssid, &devno, 0, &ccwAddr->ssid) != 0)
> +    if (virStrToLong_ui(ssid, &devno, 16, &ccwAddr->ssid) != 0)
>           return -1;
>   
>       devno++;
> -    if (virStrToLong_ui(devno, NULL, 0, &ccwAddr->devno) != 0)
> +    if (virStrToLong_ui(devno, NULL, 16, &ccwAddr->devno) != 0)
>           return -1;
>   
>       return 0;
> @@ -739,8 +739,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>           } else if (STRPREFIX((const char *)target, "hd")) {
>               if (diskAddr.type == DISK_ADDR_TYPE_IDE) {
>                   virBufferAsprintf(&buf,
> -                                  "<address type='drive' controller='%d'"
> -                                  " bus='%d' unit='%d' />\n",
> +                                  "<address type='drive' controller='%u'"
> +                                  " bus='%u' unit='%u' />\n",
>                                     diskAddr.addr.ide.controller, diskAddr.addr.ide.bus,
>                                     diskAddr.addr.ide.unit);
>               } else {

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list