Philippe Mathieu-Daudé <[email protected]> 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é <[email protected]> > --- > 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
