set_pci_host_devaddr() is hard to follow, thus bug-prone.
For example, a bug was introduced 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().
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: Rebased
v2: Free g_strsplit() with g_auto(GStrv) (Daniel)
---
hw/core/qdev-properties-system.c | 62 ++++++++++++++------------------
1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 9d80a07d26f..79408e32289 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -857,11 +857,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;
- 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);
@@ -872,58 +872,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
return;
}
- p = str;
- val = strtoul(p, &e, 16);
- if (e == p || *e != ':') {
+ col_s = col_s0 = g_strsplit(str, ":", 3);
+ if (!col_s || !col_s[0] || !col_s[1]) {
goto inval;
}
- bus = val;
- p = e + 1;
- val = strtoul(p, &e, 16);
- if (e == p) {
- goto inval;
- }
- if (*e == ':') {
- dom = bus;
- bus = val;
- p = e + 1;
- val = strtoul(p, &e, 16);
- if (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 = e + 1;
- val = strtoul(p, &e, 10);
- if (e == p) {
- goto inval;
- }
- func = val;
+ addr->bus = val;
- if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
+ /* <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
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > set_pci_host_devaddr() is hard to follow, thus bug-prone. > > For example, a bug was introduced 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(). This no longer applies to my tip of tree but in general I'm a fan. Do we have any unit tests for the qdev parsing? I couldn't see any but I'm not sure if the generic QOM tests would exercise this code. Generally when re-writing a parser it's nice to have a unit test just so you can check you've covered all the corner cases (witness the number of iterations the dfilter logic took to get right :-/). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > v3: Rebased > v2: Free g_strsplit() with g_auto(GStrv) (Daniel) > --- > hw/core/qdev-properties-system.c | 62 ++++++++++++++------------------ > 1 file changed, 27 insertions(+), 35 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 9d80a07d26f..79408e32289 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -857,11 +857,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; > - 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); > @@ -872,58 +872,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name, > return; > } > > - p = str; > - val = strtoul(p, &e, 16); > - if (e == p || *e != ':') { > + col_s = col_s0 = g_strsplit(str, ":", 3); > + if (!col_s || !col_s[0] || !col_s[1]) { I'm not sure you want max_tokens 3 because 1:2:3:4 would end up with the malformed ["1", "2", "3:4"]. You could just make your test: cols_s = g_strsplit(str, ":", -1); if (!cols_s || g_strv_length(cols_s) != 3) { error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str); return; } > goto inval; > } > - bus = val; > > - p = e + 1; > - val = strtoul(p, &e, 16); > - if (e == p) { > - goto inval; > - } > - if (*e == ':') { > - dom = bus; > - bus = val; > - p = e + 1; > - val = strtoul(p, &e, 16); > - if (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; Hmm ok PCI ids are more complex than I knew. Maybe the test above needs to be: cols_s = g_strsplit(str, ":", -1); cols_l = g_strv_length(cols_s); if (!cols_s || !(cols_l == 2 || cols_l ==3)) { error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str); return; } > > - if (*e != '.') { > + /* bus */ > + if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) { > goto inval; > } > - p = e + 1; > - val = strtoul(p, &e, 10); > - if (e == p) { > - goto inval; > - } > - func = val; > + addr->bus = val; > > - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) { > + /* <slot>.<func> */ > + dot_s = g_strsplit(col_s[1], ".", 2); > + if (!dot_s || !dot_s[0] || !dot_s[1]) { > goto inval; > } I think there is a similar length validation needed here. > > - 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 = { -- Alex Bennée
Ping? On 11/25/20 9:33 AM, Philippe Mathieu-Daudé wrote: > set_pci_host_devaddr() is hard to follow, thus bug-prone. > > For example, a bug was introduced 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(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > v3: Rebased > v2: Free g_strsplit() with g_auto(GStrv) (Daniel) > --- > hw/core/qdev-properties-system.c | 62 ++++++++++++++------------------ > 1 file changed, 27 insertions(+), 35 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 9d80a07d26f..79408e32289 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -857,11 +857,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; > - 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); > @@ -872,58 +872,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name, > return; > } > > - p = str; > - val = strtoul(p, &e, 16); > - if (e == p || *e != ':') { > + col_s = col_s0 = g_strsplit(str, ":", 3); > + if (!col_s || !col_s[0] || !col_s[1]) { > goto inval; > } > - bus = val; > > - p = e + 1; > - val = strtoul(p, &e, 16); > - if (e == p) { > - goto inval; > - } > - if (*e == ':') { > - dom = bus; > - bus = val; > - p = e + 1; > - val = strtoul(p, &e, 16); > - if (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 = e + 1; > - val = strtoul(p, &e, 10); > - if (e == p) { > - goto inval; > - } > - func = val; > + addr->bus = val; > > - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) { > + /* <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 = { >
Ping^2 On Fri, Jan 8, 2021 at 5:02 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Ping? > > On 11/25/20 9:33 AM, Philippe Mathieu-Daudé wrote: > > set_pci_host_devaddr() is hard to follow, thus bug-prone. > > > > For example, a bug was introduced 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(). > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > v3: Rebased > > v2: Free g_strsplit() with g_auto(GStrv) (Daniel) > > --- > > hw/core/qdev-properties-system.c | 62 ++++++++++++++------------------ > > 1 file changed, 27 insertions(+), 35 deletions(-) > > > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > > index 9d80a07d26f..79408e32289 100644 > > --- a/hw/core/qdev-properties-system.c > > +++ b/hw/core/qdev-properties-system.c > > @@ -857,11 +857,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; > > - 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); > > @@ -872,58 +872,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name, > > return; > > } > > > > - p = str; > > - val = strtoul(p, &e, 16); > > - if (e == p || *e != ':') { > > + col_s = col_s0 = g_strsplit(str, ":", 3); > > + if (!col_s || !col_s[0] || !col_s[1]) { > > goto inval; > > } > > - bus = val; > > > > - p = e + 1; > > - val = strtoul(p, &e, 16); > > - if (e == p) { > > - goto inval; > > - } > > - if (*e == ':') { > > - dom = bus; > > - bus = val; > > - p = e + 1; > > - val = strtoul(p, &e, 16); > > - if (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 = e + 1; > > - val = strtoul(p, &e, 10); > > - if (e == p) { > > - goto inval; > > - } > > - func = val; > > + addr->bus = val; > > > > - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) { > > + /* <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 = { > > >
© 2016 - 2024 Red Hat, Inc.