* [PATCH v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
@ 2020-10-13 10:22 Philippe Mathieu-Daudé
2020-11-07 8:59 ` [PATCH-for-5.2 " Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-13 10:22 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel P. Berrangé,
Eduardo Habkost, Richard Henderson, Klaus Herman, Paolo Bonzini,
Philippe Mathieu-Daudé
set_pci_host_devaddr() is hard to follow, thus bug-prone.
We indeed introduced a bug in commit bccb20c49df, as the
same line might be used to parse a bus (up to 0xff) or a
slot (up to 0x1f). Instead of making things worst, rewrite
using g_strsplit().
Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
Reported-by: Klaus Herman <kherman@inbox.lv>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
---
hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
1 file changed, 27 insertions(+), 34 deletions(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 49bdd125814..36d4fd8b22a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
DeviceState *dev = DEVICE(obj);
Property *prop = opaque;
PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
- char *str, *p;
- const char *e;
+ g_autofree char *str = NULL;
+ g_auto(GStrv) col_s0 = NULL;
+ g_auto(GStrv) dot_s = NULL;
+ char **col_s;
unsigned long val;
- unsigned long dom = 0, bus = 0;
- unsigned int slot = 0, func = 0;
if (dev->realized) {
qdev_prop_set_after_realize(dev, name, errp);
@@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
return;
}
- p = str;
- if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
+ col_s = col_s0 = g_strsplit(str, ":", 3);
+ if (!col_s || !col_s[0] || !col_s[1]) {
goto inval;
}
- if (*e != ':') {
- goto inval;
- }
- bus = val;
- p = (char *)e + 1;
- if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
+ /* domain */
+ if (col_s[2]) {
+ if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
goto inval;
}
+ addr->domain = val;
+ col_s++;
+ } else {
+ addr->domain = 0;
}
- slot = val;
- if (*e != '.') {
+ /* bus */
+ if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
goto inval;
}
- p = (char *)e + 1;
- if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
- goto inval;
- }
- func = val;
+ addr->bus = val;
- if (bus > 0xff) {
+ /* <slot>.<func> */
+ dot_s = g_strsplit(col_s[1], ".", 2);
+ if (!dot_s || !dot_s[0] || !dot_s[1]) {
goto inval;
}
- if (*e) {
+ /* slot */
+ if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
goto inval;
}
+ addr->slot = val;
- addr->domain = dom;
- addr->bus = bus;
- addr->slot = slot;
- addr->function = func;
+ /* func */
+ if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
+ goto inval;
+ }
+ addr->function = val;
- g_free(str);
return;
inval:
error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
- g_free(str);
}
const PropertyInfo qdev_prop_pci_host_devaddr = {
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH-for-5.2 v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
2020-10-13 10:22 [PATCH v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib Philippe Mathieu-Daudé
@ 2020-11-07 8:59 ` Philippe Mathieu-Daudé
2020-11-09 14:16 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-07 8:59 UTC (permalink / raw)
To: qemu-devel
Cc: Klaus Herman, Paolo Bonzini, Daniel P. Berrangé,
Richard Henderson, Eduardo Habkost
Ping for 5.2 as this is a bugfix.
On 10/13/20 12:22 PM, Philippe Mathieu-Daudé wrote:
> set_pci_host_devaddr() is hard to follow, thus bug-prone.
> We indeed introduced a bug in commit bccb20c49df, as the
> same line might be used to parse a bus (up to 0xff) or a
> slot (up to 0x1f). Instead of making things worst, rewrite
> using g_strsplit().
>
> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> Reported-by: Klaus Herman <kherman@inbox.lv>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
> ---
> hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
> 1 file changed, 27 insertions(+), 34 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 49bdd125814..36d4fd8b22a 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> DeviceState *dev = DEVICE(obj);
> Property *prop = opaque;
> PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> - char *str, *p;
> - const char *e;
> + g_autofree char *str = NULL;
> + g_auto(GStrv) col_s0 = NULL;
> + g_auto(GStrv) dot_s = NULL;
> + char **col_s;
> unsigned long val;
> - unsigned long dom = 0, bus = 0;
> - unsigned int slot = 0, func = 0;
>
> if (dev->realized) {
> qdev_prop_set_after_realize(dev, name, errp);
> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - p = str;
> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
> + col_s = col_s0 = g_strsplit(str, ":", 3);
> + if (!col_s || !col_s[0] || !col_s[1]) {
> goto inval;
> }
> - if (*e != ':') {
> - goto inval;
> - }
> - bus = val;
>
> - p = (char *)e + 1;
> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
> + /* domain */
> + if (col_s[2]) {
> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
> goto inval;
> }
> + addr->domain = val;
> + col_s++;
> + } else {
> + addr->domain = 0;
> }
> - slot = val;
>
> - if (*e != '.') {
> + /* bus */
> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
> goto inval;
> }
> - p = (char *)e + 1;
> - if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
> - goto inval;
> - }
> - func = val;
> + addr->bus = val;
>
> - if (bus > 0xff) {
> + /* <slot>.<func> */
> + dot_s = g_strsplit(col_s[1], ".", 2);
> + if (!dot_s || !dot_s[0] || !dot_s[1]) {
> goto inval;
> }
>
> - if (*e) {
> + /* slot */
> + if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
> goto inval;
> }
> + addr->slot = val;
>
> - addr->domain = dom;
> - addr->bus = bus;
> - addr->slot = slot;
> - addr->function = func;
> + /* func */
> + if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
> + goto inval;
> + }
> + addr->function = val;
>
> - g_free(str);
> return;
>
> inval:
> error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> - g_free(str);
> }
>
> const PropertyInfo qdev_prop_pci_host_devaddr = {
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-for-5.2 v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
2020-11-07 8:59 ` [PATCH-for-5.2 " Philippe Mathieu-Daudé
@ 2020-11-09 14:16 ` Philippe Mathieu-Daudé
2020-11-17 11:16 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-09 14:16 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Matthew Rosato,
Igor Mammedov
Cc: Klaus Herman, Paolo Bonzini, Daniel P. Berrangé,
Richard Henderson, Eduardo Habkost
Cc'ing PCI developers (rc2 is scheduled for tomorrow).
On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote:
> Ping for 5.2 as this is a bugfix.
>
> On 10/13/20 12:22 PM, Philippe Mathieu-Daudé wrote:
>> set_pci_host_devaddr() is hard to follow, thus bug-prone.
>> We indeed introduced a bug in commit bccb20c49df, as the
>> same line might be used to parse a bus (up to 0xff) or a
>> slot (up to 0x1f). Instead of making things worst, rewrite
>> using g_strsplit().
>>
>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
>> Reported-by: Klaus Herman <kherman@inbox.lv>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
>> ---
>> hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
>> 1 file changed, 27 insertions(+), 34 deletions(-)
>>
>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>> index 49bdd125814..36d4fd8b22a 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>> DeviceState *dev = DEVICE(obj);
>> Property *prop = opaque;
>> PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
>> - char *str, *p;
>> - const char *e;
>> + g_autofree char *str = NULL;
>> + g_auto(GStrv) col_s0 = NULL;
>> + g_auto(GStrv) dot_s = NULL;
>> + char **col_s;
>> unsigned long val;
>> - unsigned long dom = 0, bus = 0;
>> - unsigned int slot = 0, func = 0;
>>
>> if (dev->realized) {
>> qdev_prop_set_after_realize(dev, name, errp);
>> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>> return;
>> }
>>
>> - p = str;
>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
>> + col_s = col_s0 = g_strsplit(str, ":", 3);
>> + if (!col_s || !col_s[0] || !col_s[1]) {
>> goto inval;
>> }
>> - if (*e != ':') {
>> - goto inval;
>> - }
>> - bus = val;
>>
>> - p = (char *)e + 1;
>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
>> + /* domain */
>> + if (col_s[2]) {
>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
>> goto inval;
>> }
>> + addr->domain = val;
>> + col_s++;
>> + } else {
>> + addr->domain = 0;
>> }
>> - slot = val;
>>
>> - if (*e != '.') {
>> + /* bus */
>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
>> goto inval;
>> }
>> - p = (char *)e + 1;
>> - if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
>> - goto inval;
>> - }
>> - func = val;
>> + addr->bus = val;
>>
>> - if (bus > 0xff) {
>> + /* <slot>.<func> */
>> + dot_s = g_strsplit(col_s[1], ".", 2);
>> + if (!dot_s || !dot_s[0] || !dot_s[1]) {
>> goto inval;
>> }
>>
>> - if (*e) {
>> + /* slot */
>> + if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
>> goto inval;
>> }
>> + addr->slot = val;
>>
>> - addr->domain = dom;
>> - addr->bus = bus;
>> - addr->slot = slot;
>> - addr->function = func;
>> + /* func */
>> + if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
>> + goto inval;
>> + }
>> + addr->function = val;
>>
>> - g_free(str);
>> return;
>>
>> inval:
>> error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
>> - g_free(str);
>> }
>>
>> const PropertyInfo qdev_prop_pci_host_devaddr = {
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-for-5.2 v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
2020-11-09 14:16 ` Philippe Mathieu-Daudé
@ 2020-11-17 11:16 ` Philippe Mathieu-Daudé
2020-11-20 11:00 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-17 11:16 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Matthew Rosato,
Igor Mammedov
Cc: Klaus Herman, Paolo Bonzini, Daniel P. Berrangé,
Richard Henderson, Eduardo Habkost
ping???
On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote:
> Cc'ing PCI developers (rc2 is scheduled for tomorrow).
>
> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote:
>> Ping for 5.2 as this is a bugfix.
>>
>> On 10/13/20 12:22 PM, Philippe Mathieu-Daudé wrote:
>>> set_pci_host_devaddr() is hard to follow, thus bug-prone.
>>> We indeed introduced a bug in commit bccb20c49df, as the
>>> same line might be used to parse a bus (up to 0xff) or a
>>> slot (up to 0x1f). Instead of making things worst, rewrite
>>> using g_strsplit().
>>>
>>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
>>> Reported-by: Klaus Herman <kherman@inbox.lv>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
>>> ---
>>> hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
>>> 1 file changed, 27 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>>> index 49bdd125814..36d4fd8b22a 100644
>>> --- a/hw/core/qdev-properties-system.c
>>> +++ b/hw/core/qdev-properties-system.c
>>> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>>> DeviceState *dev = DEVICE(obj);
>>> Property *prop = opaque;
>>> PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
>>> - char *str, *p;
>>> - const char *e;
>>> + g_autofree char *str = NULL;
>>> + g_auto(GStrv) col_s0 = NULL;
>>> + g_auto(GStrv) dot_s = NULL;
>>> + char **col_s;
>>> unsigned long val;
>>> - unsigned long dom = 0, bus = 0;
>>> - unsigned int slot = 0, func = 0;
>>>
>>> if (dev->realized) {
>>> qdev_prop_set_after_realize(dev, name, errp);
>>> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>>> return;
>>> }
>>>
>>> - p = str;
>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
>>> + col_s = col_s0 = g_strsplit(str, ":", 3);
>>> + if (!col_s || !col_s[0] || !col_s[1]) {
>>> goto inval;
>>> }
>>> - if (*e != ':') {
>>> - goto inval;
>>> - }
>>> - bus = val;
>>>
>>> - p = (char *)e + 1;
>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
>>> + /* domain */
>>> + if (col_s[2]) {
>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
>>> goto inval;
>>> }
>>> + addr->domain = val;
>>> + col_s++;
>>> + } else {
>>> + addr->domain = 0;
>>> }
>>> - slot = val;
>>>
>>> - if (*e != '.') {
>>> + /* bus */
>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
>>> goto inval;
>>> }
>>> - p = (char *)e + 1;
>>> - if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
>>> - goto inval;
>>> - }
>>> - func = val;
>>> + addr->bus = val;
>>>
>>> - if (bus > 0xff) {
>>> + /* <slot>.<func> */
>>> + dot_s = g_strsplit(col_s[1], ".", 2);
>>> + if (!dot_s || !dot_s[0] || !dot_s[1]) {
>>> goto inval;
>>> }
>>>
>>> - if (*e) {
>>> + /* slot */
>>> + if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
>>> goto inval;
>>> }
>>> + addr->slot = val;
>>>
>>> - addr->domain = dom;
>>> - addr->bus = bus;
>>> - addr->slot = slot;
>>> - addr->function = func;
>>> + /* func */
>>> + if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
>>> + goto inval;
>>> + }
>>> + addr->function = val;
>>>
>>> - g_free(str);
>>> return;
>>>
>>> inval:
>>> error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
>>> - g_free(str);
>>> }
>>>
>>> const PropertyInfo qdev_prop_pci_host_devaddr = {
>>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-for-5.2 v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
2020-11-17 11:16 ` Philippe Mathieu-Daudé
@ 2020-11-20 11:00 ` Philippe Mathieu-Daudé
2020-11-20 12:55 ` Michael S. Tsirkin
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-20 11:00 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Matthew Rosato,
Igor Mammedov
Cc: Geoffrey McRae, Daniel P. Berrangé,
Eduardo Habkost, Richard Henderson, Klaus Herman, Paolo Bonzini
On 11/17/20 12:16 PM, Philippe Mathieu-Daudé wrote:
> ping???
>
> On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote:
>> Cc'ing PCI developers (rc2 is scheduled for tomorrow).
>>
>> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote:
>>> Ping for 5.2 as this is a bugfix.
>>>
>>> On 10/13/20 12:22 PM, Philippe Mathieu-Daudé wrote:
>>>> set_pci_host_devaddr() is hard to follow, thus bug-prone.
>>>> We indeed introduced a bug in commit bccb20c49df, as the
>>>> same line might be used to parse a bus (up to 0xff) or a
>>>> slot (up to 0x1f). Instead of making things worst, rewrite
>>>> using g_strsplit().
As there is few interest in fixing the issue with this patch,
let me remind the 2 other approaches:
Klaus Herman:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg750101.html
Geoffrey McRae:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg758182.html
That said, I'm done with this issue for 5.2.
Regards,
Phil.
>>>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
>>>> Reported-by: Klaus Herman <kherman@inbox.lv>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
>>>> ---
>>>> hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
>>>> 1 file changed, 27 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>>>> index 49bdd125814..36d4fd8b22a 100644
>>>> --- a/hw/core/qdev-properties-system.c
>>>> +++ b/hw/core/qdev-properties-system.c
>>>> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>>>> DeviceState *dev = DEVICE(obj);
>>>> Property *prop = opaque;
>>>> PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
>>>> - char *str, *p;
>>>> - const char *e;
>>>> + g_autofree char *str = NULL;
>>>> + g_auto(GStrv) col_s0 = NULL;
>>>> + g_auto(GStrv) dot_s = NULL;
>>>> + char **col_s;
>>>> unsigned long val;
>>>> - unsigned long dom = 0, bus = 0;
>>>> - unsigned int slot = 0, func = 0;
>>>>
>>>> if (dev->realized) {
>>>> qdev_prop_set_after_realize(dev, name, errp);
>>>> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
>>>> return;
>>>> }
>>>>
>>>> - p = str;
>>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
>>>> + col_s = col_s0 = g_strsplit(str, ":", 3);
>>>> + if (!col_s || !col_s[0] || !col_s[1]) {
>>>> goto inval;
>>>> }
>>>> - if (*e != ':') {
>>>> - goto inval;
>>>> - }
>>>> - bus = val;
>>>>
>>>> - p = (char *)e + 1;
>>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
>>>> + /* domain */
>>>> + if (col_s[2]) {
>>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
>>>> goto inval;
>>>> }
>>>> + addr->domain = val;
>>>> + col_s++;
>>>> + } else {
>>>> + addr->domain = 0;
>>>> }
>>>> - slot = val;
>>>>
>>>> - if (*e != '.') {
>>>> + /* bus */
>>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
>>>> goto inval;
>>>> }
>>>> - p = (char *)e + 1;
>>>> - if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
>>>> - goto inval;
>>>> - }
>>>> - func = val;
>>>> + addr->bus = val;
>>>>
>>>> - if (bus > 0xff) {
>>>> + /* <slot>.<func> */
>>>> + dot_s = g_strsplit(col_s[1], ".", 2);
>>>> + if (!dot_s || !dot_s[0] || !dot_s[1]) {
>>>> goto inval;
>>>> }
>>>>
>>>> - if (*e) {
>>>> + /* slot */
>>>> + if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
>>>> goto inval;
>>>> }
>>>> + addr->slot = val;
>>>>
>>>> - addr->domain = dom;
>>>> - addr->bus = bus;
>>>> - addr->slot = slot;
>>>> - addr->function = func;
>>>> + /* func */
>>>> + if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
>>>> + goto inval;
>>>> + }
>>>> + addr->function = val;
>>>>
>>>> - g_free(str);
>>>> return;
>>>>
>>>> inval:
>>>> error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
>>>> - g_free(str);
>>>> }
>>>>
>>>> const PropertyInfo qdev_prop_pci_host_devaddr = {
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-for-5.2 v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
2020-11-20 11:00 ` Philippe Mathieu-Daudé
@ 2020-11-20 12:55 ` Michael S. Tsirkin
2020-11-20 13:48 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2020-11-20 12:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Geoffrey McRae, Daniel P. Berrangé,
Eduardo Habkost, Matthew Rosato, Richard Henderson, qemu-devel,
Paolo Bonzini, Klaus Herman, Igor Mammedov
On Fri, Nov 20, 2020 at 12:00:56PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/17/20 12:16 PM, Philippe Mathieu-Daudé wrote:
> > ping???
> >
> > On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote:
> >> Cc'ing PCI developers (rc2 is scheduled for tomorrow).
> >>
> >> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote:
> >>> Ping for 5.2 as this is a bugfix.
> >>>
> >>> On 10/13/20 12:22 PM, Philippe Mathieu-Daudé wrote:
> >>>> set_pci_host_devaddr() is hard to follow, thus bug-prone.
> >>>> We indeed introduced a bug in commit bccb20c49df, as the
> >>>> same line might be used to parse a bus (up to 0xff) or a
> >>>> slot (up to 0x1f). Instead of making things worst, rewrite
> >>>> using g_strsplit().
>
> As there is few interest in fixing the issue with this patch,
> let me remind the 2 other approaches:
>
> Klaus Herman:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg750101.html
> Geoffrey McRae:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg758182.html
>
> That said, I'm done with this issue for 5.2.
>
> Regards,
>
> Phil.
What happens if we just revert bccb20c49df1bd683248a366021973901c11982f?
This is what I'm inclined to do for 5.2 ...
> >>>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> >>>> Reported-by: Klaus Herman <kherman@inbox.lv>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>> ---
> >>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
> >>>> ---
> >>>> hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
> >>>> 1 file changed, 27 insertions(+), 34 deletions(-)
> >>>>
> >>>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> >>>> index 49bdd125814..36d4fd8b22a 100644
> >>>> --- a/hw/core/qdev-properties-system.c
> >>>> +++ b/hw/core/qdev-properties-system.c
> >>>> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> >>>> DeviceState *dev = DEVICE(obj);
> >>>> Property *prop = opaque;
> >>>> PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> >>>> - char *str, *p;
> >>>> - const char *e;
> >>>> + g_autofree char *str = NULL;
> >>>> + g_auto(GStrv) col_s0 = NULL;
> >>>> + g_auto(GStrv) dot_s = NULL;
> >>>> + char **col_s;
> >>>> unsigned long val;
> >>>> - unsigned long dom = 0, bus = 0;
> >>>> - unsigned int slot = 0, func = 0;
> >>>>
> >>>> if (dev->realized) {
> >>>> qdev_prop_set_after_realize(dev, name, errp);
> >>>> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> >>>> return;
> >>>> }
> >>>>
> >>>> - p = str;
> >>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
> >>>> + col_s = col_s0 = g_strsplit(str, ":", 3);
> >>>> + if (!col_s || !col_s[0] || !col_s[1]) {
> >>>> goto inval;
> >>>> }
> >>>> - if (*e != ':') {
> >>>> - goto inval;
> >>>> - }
> >>>> - bus = val;
> >>>>
> >>>> - p = (char *)e + 1;
> >>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
> >>>> + /* domain */
> >>>> + if (col_s[2]) {
> >>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
> >>>> goto inval;
> >>>> }
> >>>> + addr->domain = val;
> >>>> + col_s++;
> >>>> + } else {
> >>>> + addr->domain = 0;
> >>>> }
> >>>> - slot = val;
> >>>>
> >>>> - if (*e != '.') {
> >>>> + /* bus */
> >>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
> >>>> goto inval;
> >>>> }
> >>>> - p = (char *)e + 1;
> >>>> - if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
> >>>> - goto inval;
> >>>> - }
> >>>> - func = val;
> >>>> + addr->bus = val;
> >>>>
> >>>> - if (bus > 0xff) {
> >>>> + /* <slot>.<func> */
> >>>> + dot_s = g_strsplit(col_s[1], ".", 2);
> >>>> + if (!dot_s || !dot_s[0] || !dot_s[1]) {
> >>>> goto inval;
> >>>> }
> >>>>
> >>>> - if (*e) {
> >>>> + /* slot */
> >>>> + if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
> >>>> goto inval;
> >>>> }
> >>>> + addr->slot = val;
> >>>>
> >>>> - addr->domain = dom;
> >>>> - addr->bus = bus;
> >>>> - addr->slot = slot;
> >>>> - addr->function = func;
> >>>> + /* func */
> >>>> + if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
> >>>> + goto inval;
> >>>> + }
> >>>> + addr->function = val;
> >>>>
> >>>> - g_free(str);
> >>>> return;
> >>>>
> >>>> inval:
> >>>> error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> >>>> - g_free(str);
> >>>> }
> >>>>
> >>>> const PropertyInfo qdev_prop_pci_host_devaddr = {
> >>>>
> >>>
> >>
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-for-5.2 v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib
2020-11-20 12:55 ` Michael S. Tsirkin
@ 2020-11-20 13:48 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-20 13:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Geoffrey McRae, Daniel P. Berrangé,
Eduardo Habkost, Matthew Rosato, Richard Henderson,
QEMU Developers, Paolo Bonzini, Klaus Herman, Igor Mammedov
On Fri, Nov 20, 2020 at 1:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Nov 20, 2020 at 12:00:56PM +0100, Philippe Mathieu-Daudé wrote:
> > On 11/17/20 12:16 PM, Philippe Mathieu-Daudé wrote:
> > > ping???
> > >
> > > On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote:
> > >> Cc'ing PCI developers (rc2 is scheduled for tomorrow).
> > >>
> > >> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote:
> > >>> Ping for 5.2 as this is a bugfix.
> > >>>
> > >>> On 10/13/20 12:22 PM, Philippe Mathieu-Daudé wrote:
> > >>>> set_pci_host_devaddr() is hard to follow, thus bug-prone.
> > >>>> We indeed introduced a bug in commit bccb20c49df, as the
> > >>>> same line might be used to parse a bus (up to 0xff) or a
> > >>>> slot (up to 0x1f). Instead of making things worst, rewrite
> > >>>> using g_strsplit().
> >
> > As there is few interest in fixing the issue with this patch,
> > let me remind the 2 other approaches:
> >
> > Klaus Herman:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg750101.html
> > Geoffrey McRae:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg758182.html
> >
> > That said, I'm done with this issue for 5.2.
> >
> > Regards,
> >
> > Phil.
>
> What happens if we just revert bccb20c49df1bd683248a366021973901c11982f?
>
> This is what I'm inclined to do for 5.2 ...
Clever, fine by me.
> > >>>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
> > >>>> Reported-by: Klaus Herman <kherman@inbox.lv>
> > >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > >>>> ---
> > >>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
> > >>>> ---
> > >>>> hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
> > >>>> 1 file changed, 27 insertions(+), 34 deletions(-)
> > >>>>
> > >>>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> > >>>> index 49bdd125814..36d4fd8b22a 100644
> > >>>> --- a/hw/core/qdev-properties-system.c
> > >>>> +++ b/hw/core/qdev-properties-system.c
> > >>>> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> > >>>> DeviceState *dev = DEVICE(obj);
> > >>>> Property *prop = opaque;
> > >>>> PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> > >>>> - char *str, *p;
> > >>>> - const char *e;
> > >>>> + g_autofree char *str = NULL;
> > >>>> + g_auto(GStrv) col_s0 = NULL;
> > >>>> + g_auto(GStrv) dot_s = NULL;
> > >>>> + char **col_s;
> > >>>> unsigned long val;
> > >>>> - unsigned long dom = 0, bus = 0;
> > >>>> - unsigned int slot = 0, func = 0;
> > >>>>
> > >>>> if (dev->realized) {
> > >>>> qdev_prop_set_after_realize(dev, name, errp);
> > >>>> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> > >>>> return;
> > >>>> }
> > >>>>
> > >>>> - p = str;
> > >>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
> > >>>> + col_s = col_s0 = g_strsplit(str, ":", 3);
> > >>>> + if (!col_s || !col_s[0] || !col_s[1]) {
> > >>>> goto inval;
> > >>>> }
> > >>>> - if (*e != ':') {
> > >>>> - goto inval;
> > >>>> - }
> > >>>> - bus = val;
> > >>>>
> > >>>> - p = (char *)e + 1;
> > >>>> - if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || 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) {
> > >>>> + /* domain */
> > >>>> + if (col_s[2]) {
> > >>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
> > >>>> goto inval;
> > >>>> }
> > >>>> + addr->domain = val;
> > >>>> + col_s++;
> > >>>> + } else {
> > >>>> + addr->domain = 0;
> > >>>> }
> > >>>> - slot = val;
> > >>>>
> > >>>> - if (*e != '.') {
> > >>>> + /* bus */
> > >>>> + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
> > >>>> goto inval;
> > >>>> }
> > >>>> - p = (char *)e + 1;
> > >>>> - if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
> > >>>> - goto inval;
> > >>>> - }
> > >>>> - func = val;
> > >>>> + addr->bus = val;
> > >>>>
> > >>>> - if (bus > 0xff) {
> > >>>> + /* <slot>.<func> */
> > >>>> + dot_s = g_strsplit(col_s[1], ".", 2);
> > >>>> + if (!dot_s || !dot_s[0] || !dot_s[1]) {
> > >>>> goto inval;
> > >>>> }
> > >>>>
> > >>>> - if (*e) {
> > >>>> + /* slot */
> > >>>> + if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
> > >>>> goto inval;
> > >>>> }
> > >>>> + addr->slot = val;
> > >>>>
> > >>>> - addr->domain = dom;
> > >>>> - addr->bus = bus;
> > >>>> - addr->slot = slot;
> > >>>> - addr->function = func;
> > >>>> + /* func */
> > >>>> + if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
> > >>>> + goto inval;
> > >>>> + }
> > >>>> + addr->function = val;
> > >>>>
> > >>>> - g_free(str);
> > >>>> return;
> > >>>>
> > >>>> inval:
> > >>>> error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > >>>> - g_free(str);
> > >>>> }
> > >>>>
> > >>>> const PropertyInfo qdev_prop_pci_host_devaddr = {
> > >>>>
> > >>>
> > >>
> > >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-20 13:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-13 10:22 [PATCH v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib Philippe Mathieu-Daudé
2020-11-07 8:59 ` [PATCH-for-5.2 " Philippe Mathieu-Daudé
2020-11-09 14:16 ` Philippe Mathieu-Daudé
2020-11-17 11:16 ` Philippe Mathieu-Daudé
2020-11-20 11:00 ` Philippe Mathieu-Daudé
2020-11-20 12:55 ` Michael S. Tsirkin
2020-11-20 13:48 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.