[PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[Michael S. Tsirkin]

This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
introduced a regression blocking bus addresses > 0x1f or higher.
Legal bus numbers go up to 0xff.

Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
Reported-by: Klaus Herman <kherman@inbox.lv>
Reported-by: Geoffrey McRae <geoff@hostfission.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

checkpatch will complain since it does not like strtoul but
we had it for years so should be ok for 5.2, right?

 hw/core/qdev-properties-system.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index b81a4e8d14..9d80a07d26 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -858,7 +858,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
     Property *prop = opaque;
     PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
     char *str, *p;
-    const char *e;
+    char *e;
     unsigned long val;
     unsigned long dom = 0, bus = 0;
     unsigned int slot = 0, func = 0;
@@ -873,23 +873,23 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
     }
 
     p = str;
-    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
-        goto inval;
-    }
-    if (*e != ':') {
+    val = strtoul(p, &e, 16);
+    if (e == p || *e != ':') {
         goto inval;
     }
     bus = val;
 
-    p = (char *)e + 1;
-    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
+    p = e + 1;
+    val = strtoul(p, &e, 16);
+    if (e == p) {
         goto inval;
     }
     if (*e == ':') {
         dom = bus;
         bus = val;
-        p = (char *)e + 1;
-        if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
+        p = e + 1;
+        val = strtoul(p, &e, 16);
+        if (e == p) {
             goto inval;
         }
     }
@@ -898,13 +898,14 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
     if (*e != '.') {
         goto inval;
     }
-    p = (char *)e + 1;
-    if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
+    p = e + 1;
+    val = strtoul(p, &e, 10);
+    if (e == p) {
         goto inval;
     }
     func = val;
 
-    if (bus > 0xff) {
+    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
         goto inval;
     }
 
-- 
MST

Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201120130409.956956-1-mst@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201120130409.956956-1-mst@redhat.com
Subject: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20201120073149.99079-1-pbonzini@redhat.com -> patchew/20201120073149.99079-1-pbonzini@redhat.com
 * [new tag]         patchew/20201120130409.956956-1-mst@redhat.com -> patchew/20201120130409.956956-1-mst@redhat.com
Switched to a new branch 'test'
1d9f6bd Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

=== OUTPUT BEGIN ===
ERROR: consider using qemu_strtoul in preference to strtoul
#39: FILE: hw/core/qdev-properties-system.c:876:
+    val = strtoul(p, &e, 16);

ERROR: consider using qemu_strtoul in preference to strtoul
#48: FILE: hw/core/qdev-properties-system.c:883:
+    val = strtoul(p, &e, 16);

ERROR: consider using qemu_strtoul in preference to strtoul
#58: FILE: hw/core/qdev-properties-system.c:891:
+        val = strtoul(p, &e, 16);

ERROR: consider using qemu_strtoul in preference to strtoul
#70: FILE: hw/core/qdev-properties-system.c:902:
+    val = strtoul(p, &e, 10);

total: 4 errors, 0 warnings, 56 lines checked

Commit 1d9f6bd1223f (Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()") has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201120130409.956956-1-mst@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[Paolo Bonzini]

On 20/11/20 14:04, Michael S. Tsirkin wrote:
> This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
> introduced a regression blocking bus addresses > 0x1f or higher.
> Legal bus numbers go up to 0xff.
> 
> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> Reported-by: Klaus Herman <kherman@inbox.lv>
> Reported-by: Geoffrey McRae <geoff@hostfission.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> checkpatch will complain since it does not like strtoul but
> we had it for years so should be ok for 5.2, right?

Yes, of course.

Paolo

>   hw/core/qdev-properties-system.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index b81a4e8d14..9d80a07d26 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -858,7 +858,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>       Property *prop = opaque;
>       PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
>       char *str, *p;
> -    const char *e;
> +    char *e;
>       unsigned long val;
>       unsigned long dom = 0, bus = 0;
>       unsigned int slot = 0, func = 0;
> @@ -873,23 +873,23 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>       }
>   
>       p = str;
> -    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
> -        goto inval;
> -    }
> -    if (*e != ':') {
> +    val = strtoul(p, &e, 16);
> +    if (e == p || *e != ':') {
>           goto inval;
>       }
>       bus = val;
>   
> -    p = (char *)e + 1;
> -    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
> +    p = e + 1;
> +    val = strtoul(p, &e, 16);
> +    if (e == p) {
>           goto inval;
>       }
>       if (*e == ':') {
>           dom = bus;
>           bus = val;
> -        p = (char *)e + 1;
> -        if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
> +        p = e + 1;
> +        val = strtoul(p, &e, 16);
> +        if (e == p) {
>               goto inval;
>           }
>       }
> @@ -898,13 +898,14 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>       if (*e != '.') {
>           goto inval;
>       }
> -    p = (char *)e + 1;
> -    if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
> +    p = e + 1;
> +    val = strtoul(p, &e, 10);
> +    if (e == p) {
>           goto inval;
>       }
>       func = val;
>   
> -    if (bus > 0xff) {
> +    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
>           goto inval;
>       }
>   
> 

Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[Philippe Mathieu-Daudé]

On 11/20/20 2:04 PM, Michael S. Tsirkin wrote:
> This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
> introduced a regression blocking bus addresses > 0x1f or higher.
> Legal bus numbers go up to 0xff.
> 
> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> Reported-by: Klaus Herman <kherman@inbox.lv>
> Reported-by: Geoffrey McRae <geoff@hostfission.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> checkpatch will complain since it does not like strtoul but
> we had it for years so should be ok for 5.2, right?
> 
>  hw/core/qdev-properties-system.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)

Thanks!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[Philippe Mathieu-Daudé]

On 11/20/20 2:10 PM, Paolo Bonzini wrote:
> On 20/11/20 14:04, Michael S. Tsirkin wrote:
>> This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
>> introduced a regression blocking bus addresses > 0x1f or higher.
>> Legal bus numbers go up to 0xff.
>>
>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
>> Reported-by: Klaus Herman <kherman@inbox.lv>
>> Reported-by: Geoffrey McRae <geoff@hostfission.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>
>> checkpatch will complain since it does not like strtoul but
>> we had it for years so should be ok for 5.2, right?
> 
> Yes, of course.
> 
> Paolo

Which tree is going to merge this patch?

Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[Eduardo Habkost]

On Tue, Nov 24, 2020 at 03:13:14PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/20/20 2:10 PM, Paolo Bonzini wrote:
> > On 20/11/20 14:04, Michael S. Tsirkin wrote:
> >> This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
> >> introduced a regression blocking bus addresses > 0x1f or higher.
> >> Legal bus numbers go up to 0xff.
> >>
> >> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> >> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> >> Reported-by: Klaus Herman <kherman@inbox.lv>
> >> Reported-by: Geoffrey McRae <geoff@hostfission.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>
> >> checkpatch will complain since it does not like strtoul but
> >> we had it for years so should be ok for 5.2, right?
> > 
> > Yes, of course.
> > 
> > Paolo
> 
> Which tree is going to merge this patch?

Sorry, I was expecting Michael to merge it, as it's PCI-specific
but it looks like people were waiting for me to merge it.

I'll merge it and submit a pull request ASAP.

-- 
Eduardo

Re: [PATCH for-5.2] Revert "hw/core/qdev-properties: Use qemu_strtoul() in set_pci_host_devaddr()"

[Philippe Mathieu-Daudé]

On 11/24/20 4:06 PM, Eduardo Habkost wrote:
> On Tue, Nov 24, 2020 at 03:13:14PM +0100, Philippe Mathieu-Daudé wrote:
>> On 11/20/20 2:10 PM, Paolo Bonzini wrote:
>>> On 20/11/20 14:04, Michael S. Tsirkin wrote:
>>>> This reverts commit bccb20c49df1bd683248a366021973901c11982f as it
>>>> introduced a regression blocking bus addresses > 0x1f or higher.
>>>> Legal bus numbers go up to 0xff.
>>>>
>>>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
>>>> Reported-by: Klaus Herman <kherman@inbox.lv>
>>>> Reported-by: Geoffrey McRae <geoff@hostfission.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>
>>>> checkpatch will complain since it does not like strtoul but
>>>> we had it for years so should be ok for 5.2, right?
>>>
>>> Yes, of course.
>>>
>>> Paolo
>>
>> Which tree is going to merge this patch?
> 
> Sorry, I was expecting Michael to merge it, as it's PCI-specific
> but it looks like people were waiting for me to merge it.
> 
> I'll merge it and submit a pull request ASAP.

Thanks!