* [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
@ 2017-02-16 10:58 Marc-André Lureau
2017-02-16 11:13 ` Marcel Apfelbaum
2017-02-16 14:37 ` Michael S. Tsirkin
0 siblings, 2 replies; 8+ messages in thread
From: Marc-André Lureau @ 2017-02-16 10:58 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, Marc-André Lureau
This will allow the function to be used outside of QOM properties.
Add tests and replace strtoul usage for qemu_strtoul while touching it.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
v2:
- fixed incompatible type warning/error
include/hw/pci/pci.h | 12 ++++++++++
hw/core/qdev-properties.c | 61 +++--------------------------------------------
tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
util/cutils.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 120 insertions(+), 58 deletions(-)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index cbc1fdfb5b..61718080ea 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
+/**
+ * pci_host_device_address_parse:
+ * @str: string to parse
+ * @addr: destination structure or NULL
+ *
+ * Parse [<domain>:]<bus>:<slot>.<func>
+ * if <domain> is not supplied, it's assumed to be 0.
+ *
+ * Returns: -1 on failure, 0 on success.
+ */
+int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress *addr);
+
#endif
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 6ab4265eb4..5dd0774e88 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
visit_type_str(v, name, &p, errp);
}
-/*
- * Parse [<domain>:]<bus>:<slot>.<func>
- * if <domain> is not supplied, it's assumed to be 0.
- */
static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -733,11 +729,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);
Error *local_err = NULL;
- char *str, *p;
- char *e;
- unsigned long val;
- unsigned long dom = 0, bus = 0;
- unsigned int slot = 0, func = 0;
+ char *str;
if (dev->realized) {
qdev_prop_set_after_realize(dev, name, errp);
@@ -750,58 +742,11 @@ 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 != ':') {
- 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) {
- goto inval;
- }
- }
- slot = val;
-
- if (*e != '.') {
- goto inval;
+ if (!pci_host_device_address_parse(str, addr)) {
+ error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
}
- p = e + 1;
- val = strtoul(p, &e, 10);
- if (e == p) {
- goto inval;
- }
- func = val;
-
- if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
- goto inval;
- }
-
- if (*e) {
- goto inval;
- }
-
- addr->domain = dom;
- addr->bus = bus;
- addr->slot = slot;
- addr->function = func;
g_free(str);
- return;
-
-inval:
- error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
- g_free(str);
}
PropertyInfo qdev_prop_pci_host_devaddr = {
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 20b0f59ba2..81c4f072f3 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -28,6 +28,7 @@
#include "qemu/osdep.h"
#include "qemu/cutils.h"
+#include "hw/pci/pci.h"
static void test_parse_uint_null(void)
{
@@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
g_assert_cmpint(res, ==, 12345000);
}
+static void test_pci_parse_valid(void)
+{
+ PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
+ int i, res;
+ const struct {
+ const char *str;
+ PCIHostDeviceAddress addr;
+ } tests[] = {
+ { "1:2.3", { 0, 1, 2, 3 } },
+ { "4:1:2.3", { 4, 1, 2, 3 } },
+ { "a:a:a.7", { 10, 10, 10, 7 } },
+ };
+
+ for (i = 0; i < ARRAY_SIZE(tests); i++) {
+ res = pci_host_device_address_parse(tests[i].str, &addr);
+ g_assert_cmpint(res, ==, 0);
+
+ g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
+ g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
+ g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
+ g_assert_cmpint(addr.function, ==, tests[i].addr.function);
+ }
+}
+
+static void test_pci_parse_invalid(void)
+{
+ int i, res;
+ const char *tests[] = {
+ "", "foo", "1:2.3 foo",
+ "a:a:a.a",
+ "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
+ "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
+ "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
+
+ for (i = 0; i < ARRAY_SIZE(tests); i++) {
+ res = pci_host_device_address_parse(tests[i], NULL);
+ g_assert_cmpint(res, ==, -1);
+ }
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
g_test_add_func("/cutils/strtosz/suffix-unit",
test_qemu_strtosz_suffix_unit);
+ g_test_add_func("/cutils/pci-parse/valid",
+ test_pci_parse_valid);
+ g_test_add_func("/cutils/pci-parse/invalid",
+ test_pci_parse_invalid);
+
return g_test_run();
}
diff --git a/util/cutils.c b/util/cutils.c
index 4fefcf3be3..fc71c85816 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -29,6 +29,7 @@
#include "qemu/sockets.h"
#include "qemu/iov.h"
#include "net/net.h"
+#include "hw/pci/pci.h"
#include "qemu/cutils.h"
void strpadcpy(char *buf, int buf_size, const char *str, char pad)
@@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
return ret;
}
+
+int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress *addr)
+{
+ const char *p = str;
+ const char *e;
+ unsigned long val;
+ unsigned int dom = 0, bus = 0;
+ unsigned int slot = 0, func = 0;
+
+ if (qemu_strtoul(p, &e, 16, &val) < 0
+ || e == p || *e != ':') {
+ return -1;
+ }
+ bus = val;
+
+ p = e + 1;
+ if (qemu_strtoul(p, &e, 16, &val) < 0
+ || e == p) {
+ return -1;
+ }
+ if (*e == ':') {
+ dom = bus;
+ bus = val;
+ p = e + 1;
+ if (qemu_strtoul(p, &e, 16, &val) < 0
+ || e == p) {
+ return -1;
+ }
+ }
+ slot = val;
+
+ if (*e != '.') {
+ return -1;
+ }
+ p = e + 1;
+ if (qemu_strtoul(p, &e, 10, &val) < 0
+ || e == p) {
+ return -1;
+ }
+ func = val;
+
+ if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
+ return -1;
+ }
+
+ if (*e) {
+ return -1;
+ }
+
+ if (addr) {
+ addr->domain = dom;
+ addr->bus = bus;
+ addr->slot = slot;
+ addr->function = func;
+ }
+
+ return 0;
+}
--
2.11.0.295.gd7dffce1c.dirty
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 10:58 [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse() Marc-André Lureau
@ 2017-02-16 11:13 ` Marcel Apfelbaum
2017-02-16 11:19 ` Marc-André Lureau
2017-02-16 14:37 ` Michael S. Tsirkin
1 sibling, 1 reply; 8+ messages in thread
From: Marcel Apfelbaum @ 2017-02-16 11:13 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: mst
On 02/16/2017 12:58 PM, Marc-André Lureau wrote:
> This will allow the function to be used outside of QOM properties.
>
Hi Mark-Andre,
I am not sure cutil.h is the right header for the function, because is too PCI
related for a utility library.
Also, maybe we can post the patch together with the code that uses the exposed function.
And thanks for the unit-tests, much appreciated!
Marcel
> Add tests and replace strtoul usage for qemu_strtoul while touching it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> v2:
> - fixed incompatible type warning/error
>
> include/hw/pci/pci.h | 12 ++++++++++
> hw/core/qdev-properties.c | 61 +++--------------------------------------------
> tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
> util/cutils.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 120 insertions(+), 58 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index cbc1fdfb5b..61718080ea 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
>
> MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
>
> +/**
> + * pci_host_device_address_parse:
> + * @str: string to parse
> + * @addr: destination structure or NULL
> + *
> + * Parse [<domain>:]<bus>:<slot>.<func>
> + * if <domain> is not supplied, it's assumed to be 0.
> + *
> + * Returns: -1 on failure, 0 on success.
> + */
> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress *addr);
> +
> #endif
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 6ab4265eb4..5dd0774e88 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> visit_type_str(v, name, &p, errp);
> }
>
> -/*
> - * Parse [<domain>:]<bus>:<slot>.<func>
> - * if <domain> is not supplied, it's assumed to be 0.
> - */
> static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -733,11 +729,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);
> Error *local_err = NULL;
> - char *str, *p;
> - char *e;
> - unsigned long val;
> - unsigned long dom = 0, bus = 0;
> - unsigned int slot = 0, func = 0;
> + char *str;
>
> if (dev->realized) {
> qdev_prop_set_after_realize(dev, name, errp);
> @@ -750,58 +742,11 @@ 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 != ':') {
> - 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) {
> - goto inval;
> - }
> - }
> - slot = val;
> -
> - if (*e != '.') {
> - goto inval;
> + if (!pci_host_device_address_parse(str, addr)) {
> + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> }
> - p = e + 1;
> - val = strtoul(p, &e, 10);
> - if (e == p) {
> - goto inval;
> - }
> - func = val;
> -
> - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> - goto inval;
> - }
> -
> - if (*e) {
> - goto inval;
> - }
> -
> - addr->domain = dom;
> - addr->bus = bus;
> - addr->slot = slot;
> - addr->function = func;
>
> g_free(str);
> - return;
> -
> -inval:
> - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> - g_free(str);
> }
>
> PropertyInfo qdev_prop_pci_host_devaddr = {
> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> index 20b0f59ba2..81c4f072f3 100644
> --- a/tests/test-cutils.c
> +++ b/tests/test-cutils.c
> @@ -28,6 +28,7 @@
> #include "qemu/osdep.h"
>
> #include "qemu/cutils.h"
> +#include "hw/pci/pci.h"
>
> static void test_parse_uint_null(void)
> {
> @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
> g_assert_cmpint(res, ==, 12345000);
> }
>
> +static void test_pci_parse_valid(void)
> +{
> + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
> + int i, res;
> + const struct {
> + const char *str;
> + PCIHostDeviceAddress addr;
> + } tests[] = {
> + { "1:2.3", { 0, 1, 2, 3 } },
> + { "4:1:2.3", { 4, 1, 2, 3 } },
> + { "a:a:a.7", { 10, 10, 10, 7 } },
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> + res = pci_host_device_address_parse(tests[i].str, &addr);
> + g_assert_cmpint(res, ==, 0);
> +
> + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
> + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
> + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
> + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
> + }
> +}
> +
> +static void test_pci_parse_invalid(void)
> +{
> + int i, res;
> + const char *tests[] = {
> + "", "foo", "1:2.3 foo",
> + "a:a:a.a",
> + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
> + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
> + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
> +
> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> + res = pci_host_device_address_parse(tests[i], NULL);
> + g_assert_cmpint(res, ==, -1);
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
> g_test_add_func("/cutils/strtosz/suffix-unit",
> test_qemu_strtosz_suffix_unit);
>
> + g_test_add_func("/cutils/pci-parse/valid",
> + test_pci_parse_valid);
> + g_test_add_func("/cutils/pci-parse/invalid",
> + test_pci_parse_invalid);
> +
> return g_test_run();
> }
> diff --git a/util/cutils.c b/util/cutils.c
> index 4fefcf3be3..fc71c85816 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -29,6 +29,7 @@
> #include "qemu/sockets.h"
> #include "qemu/iov.h"
> #include "net/net.h"
> +#include "hw/pci/pci.h"
> #include "qemu/cutils.h"
>
> void strpadcpy(char *buf, int buf_size, const char *str, char pad)
> @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
>
> return ret;
> }
> +
> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress *addr)
> +{
> + const char *p = str;
> + const char *e;
> + unsigned long val;
> + unsigned int dom = 0, bus = 0;
> + unsigned int slot = 0, func = 0;
> +
> + if (qemu_strtoul(p, &e, 16, &val) < 0
> + || e == p || *e != ':') {
> + return -1;
> + }
> + bus = val;
> +
> + p = e + 1;
> + if (qemu_strtoul(p, &e, 16, &val) < 0
> + || e == p) {
> + return -1;
> + }
> + if (*e == ':') {
> + dom = bus;
> + bus = val;
> + p = e + 1;
> + if (qemu_strtoul(p, &e, 16, &val) < 0
> + || e == p) {
> + return -1;
> + }
> + }
> + slot = val;
> +
> + if (*e != '.') {
> + return -1;
> + }
> + p = e + 1;
> + if (qemu_strtoul(p, &e, 10, &val) < 0
> + || e == p) {
> + return -1;
> + }
> + func = val;
> +
> + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> + return -1;
> + }
> +
> + if (*e) {
> + return -1;
> + }
> +
> + if (addr) {
> + addr->domain = dom;
> + addr->bus = bus;
> + addr->slot = slot;
> + addr->function = func;
> + }
> +
> + return 0;
> +}
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 11:13 ` Marcel Apfelbaum
@ 2017-02-16 11:19 ` Marc-André Lureau
2017-02-16 11:30 ` Marcel Apfelbaum
0 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2017-02-16 11:19 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: Marc-André Lureau, qemu-devel, mst
Hi
----- Original Message -----
> On 02/16/2017 12:58 PM, Marc-André Lureau wrote:
> > This will allow the function to be used outside of QOM properties.
> >
>
> Hi Mark-Andre,
>
> I am not sure cutil.h is the right header for the function, because is too
> PCI
> related for a utility library.
It's in the pci.h header, implementation and test is put in {test-,}cutils.c
I can try to put it in pci/pci.c, but it will be harder to link to for testing (note: I haven't tried). I could also put it in a seperate pci-parse.c for example.
> Also, maybe we can post the patch together with the code that uses the
> exposed function.
I dropped the patch using it for now, but I thought it would still be nice to have the test, doc and strtoul change.
>
> And thanks for the unit-tests, much appreciated!
thanks
> Marcel
>
> > Add tests and replace strtoul usage for qemu_strtoul while touching it.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > v2:
> > - fixed incompatible type warning/error
> >
> > include/hw/pci/pci.h | 12 ++++++++++
> > hw/core/qdev-properties.c | 61
> > +++--------------------------------------------
> > tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
> > util/cutils.c | 59
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 120 insertions(+), 58 deletions(-)
> >
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index cbc1fdfb5b..61718080ea 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
> >
> > MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> >
> > +/**
> > + * pci_host_device_address_parse:
> > + * @str: string to parse
> > + * @addr: destination structure or NULL
> > + *
> > + * Parse [<domain>:]<bus>:<slot>.<func>
> > + * if <domain> is not supplied, it's assumed to be 0.
> > + *
> > + * Returns: -1 on failure, 0 on success.
> > + */
> > +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> > *addr);
> > +
> > #endif
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 6ab4265eb4..5dd0774e88 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor
> > *v, const char *name,
> > visit_type_str(v, name, &p, errp);
> > }
> >
> > -/*
> > - * Parse [<domain>:]<bus>:<slot>.<func>
> > - * if <domain> is not supplied, it's assumed to be 0.
> > - */
> > static void set_pci_host_devaddr(Object *obj, Visitor *v, const char
> > *name,
> > void *opaque, Error **errp)
> > {
> > @@ -733,11 +729,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);
> > Error *local_err = NULL;
> > - char *str, *p;
> > - char *e;
> > - unsigned long val;
> > - unsigned long dom = 0, bus = 0;
> > - unsigned int slot = 0, func = 0;
> > + char *str;
> >
> > if (dev->realized) {
> > qdev_prop_set_after_realize(dev, name, errp);
> > @@ -750,58 +742,11 @@ 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 != ':') {
> > - 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) {
> > - goto inval;
> > - }
> > - }
> > - slot = val;
> > -
> > - if (*e != '.') {
> > - goto inval;
> > + if (!pci_host_device_address_parse(str, addr)) {
> > + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > }
> > - p = e + 1;
> > - val = strtoul(p, &e, 10);
> > - if (e == p) {
> > - goto inval;
> > - }
> > - func = val;
> > -
> > - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> > - goto inval;
> > - }
> > -
> > - if (*e) {
> > - goto inval;
> > - }
> > -
> > - addr->domain = dom;
> > - addr->bus = bus;
> > - addr->slot = slot;
> > - addr->function = func;
> >
> > g_free(str);
> > - return;
> > -
> > -inval:
> > - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > - g_free(str);
> > }
> >
> > PropertyInfo qdev_prop_pci_host_devaddr = {
> > diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> > index 20b0f59ba2..81c4f072f3 100644
> > --- a/tests/test-cutils.c
> > +++ b/tests/test-cutils.c
> > @@ -28,6 +28,7 @@
> > #include "qemu/osdep.h"
> >
> > #include "qemu/cutils.h"
> > +#include "hw/pci/pci.h"
> >
> > static void test_parse_uint_null(void)
> > {
> > @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
> > g_assert_cmpint(res, ==, 12345000);
> > }
> >
> > +static void test_pci_parse_valid(void)
> > +{
> > + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
> > + int i, res;
> > + const struct {
> > + const char *str;
> > + PCIHostDeviceAddress addr;
> > + } tests[] = {
> > + { "1:2.3", { 0, 1, 2, 3 } },
> > + { "4:1:2.3", { 4, 1, 2, 3 } },
> > + { "a:a:a.7", { 10, 10, 10, 7 } },
> > + };
> > +
> > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > + res = pci_host_device_address_parse(tests[i].str, &addr);
> > + g_assert_cmpint(res, ==, 0);
> > +
> > + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
> > + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
> > + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
> > + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
> > + }
> > +}
> > +
> > +static void test_pci_parse_invalid(void)
> > +{
> > + int i, res;
> > + const char *tests[] = {
> > + "", "foo", "1:2.3 foo",
> > + "a:a:a.a",
> > + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
> > + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
> > + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
> > +
> > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > + res = pci_host_device_address_parse(tests[i], NULL);
> > + g_assert_cmpint(res, ==, -1);
> > + }
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > g_test_init(&argc, &argv, NULL);
> > @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
> > g_test_add_func("/cutils/strtosz/suffix-unit",
> > test_qemu_strtosz_suffix_unit);
> >
> > + g_test_add_func("/cutils/pci-parse/valid",
> > + test_pci_parse_valid);
> > + g_test_add_func("/cutils/pci-parse/invalid",
> > + test_pci_parse_invalid);
> > +
> > return g_test_run();
> > }
> > diff --git a/util/cutils.c b/util/cutils.c
> > index 4fefcf3be3..fc71c85816 100644
> > --- a/util/cutils.c
> > +++ b/util/cutils.c
> > @@ -29,6 +29,7 @@
> > #include "qemu/sockets.h"
> > #include "qemu/iov.h"
> > #include "net/net.h"
> > +#include "hw/pci/pci.h"
> > #include "qemu/cutils.h"
> >
> > void strpadcpy(char *buf, int buf_size, const char *str, char pad)
> > @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
> >
> > return ret;
> > }
> > +
> > +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> > *addr)
> > +{
> > + const char *p = str;
> > + const char *e;
> > + unsigned long val;
> > + unsigned int dom = 0, bus = 0;
> > + unsigned int slot = 0, func = 0;
> > +
> > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > + || e == p || *e != ':') {
> > + return -1;
> > + }
> > + bus = val;
> > +
> > + p = e + 1;
> > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > + || e == p) {
> > + return -1;
> > + }
> > + if (*e == ':') {
> > + dom = bus;
> > + bus = val;
> > + p = e + 1;
> > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > + || e == p) {
> > + return -1;
> > + }
> > + }
> > + slot = val;
> > +
> > + if (*e != '.') {
> > + return -1;
> > + }
> > + p = e + 1;
> > + if (qemu_strtoul(p, &e, 10, &val) < 0
> > + || e == p) {
> > + return -1;
> > + }
> > + func = val;
> > +
> > + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> > + return -1;
> > + }
> > +
> > + if (*e) {
> > + return -1;
> > + }
> > +
> > + if (addr) {
> > + addr->domain = dom;
> > + addr->bus = bus;
> > + addr->slot = slot;
> > + addr->function = func;
> > + }
> > +
> > + return 0;
> > +}
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 11:19 ` Marc-André Lureau
@ 2017-02-16 11:30 ` Marcel Apfelbaum
2017-02-16 12:08 ` Marc-André Lureau
0 siblings, 1 reply; 8+ messages in thread
From: Marcel Apfelbaum @ 2017-02-16 11:30 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Marc-André Lureau, qemu-devel, mst
On 02/16/2017 01:19 PM, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 02/16/2017 12:58 PM, Marc-André Lureau wrote:
>>> This will allow the function to be used outside of QOM properties.
>>>
>>
>> Hi Mark-Andre,
>>
>> I am not sure cutil.h is the right header for the function, because is too
>> PCI
>> related for a utility library.
>
> It's in the pci.h header, implementation and test is put in {test-,}cutils.c
>
sorry about the miss-match, I war referring to the implementation.
> I can try to put it in pci/pci.c, but it will be harder to link to for testing (note: I haven't tried). I could also put it in a seperate pci-parse.c for example.
no need for a new file, but hw/pci/pci_host.c should be a good place.
regarding the tests, an new pci test file would be welcomed.
>
>
>> Also, maybe we can post the patch together with the code that uses the
>> exposed function.
>
> I dropped the patch using it for now, but I thought it would still be nice to have the test, doc and strtoul change.
>
I agree if Michael has nothing against this.
Thanks,
Marcel
>>
>> And thanks for the unit-tests, much appreciated!
>
> thanks
>
>> Marcel
>>
>>> Add tests and replace strtoul usage for qemu_strtoul while touching it.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>> v2:
>>> - fixed incompatible type warning/error
>>>
>>> include/hw/pci/pci.h | 12 ++++++++++
>>> hw/core/qdev-properties.c | 61
>>> +++--------------------------------------------
>>> tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
>>> util/cutils.c | 59
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 120 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>> index cbc1fdfb5b..61718080ea 100644
>>> --- a/include/hw/pci/pci.h
>>> +++ b/include/hw/pci/pci.h
>>> @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
>>>
>>> MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
>>>
>>> +/**
>>> + * pci_host_device_address_parse:
>>> + * @str: string to parse
>>> + * @addr: destination structure or NULL
>>> + *
>>> + * Parse [<domain>:]<bus>:<slot>.<func>
>>> + * if <domain> is not supplied, it's assumed to be 0.
>>> + *
>>> + * Returns: -1 on failure, 0 on success.
>>> + */
>>> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
>>> *addr);
>>> +
>>> #endif
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 6ab4265eb4..5dd0774e88 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor
>>> *v, const char *name,
>>> visit_type_str(v, name, &p, errp);
>>> }
>>>
>>> -/*
>>> - * Parse [<domain>:]<bus>:<slot>.<func>
>>> - * if <domain> is not supplied, it's assumed to be 0.
>>> - */
>>> static void set_pci_host_devaddr(Object *obj, Visitor *v, const char
>>> *name,
>>> void *opaque, Error **errp)
>>> {
>>> @@ -733,11 +729,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);
>>> Error *local_err = NULL;
>>> - char *str, *p;
>>> - char *e;
>>> - unsigned long val;
>>> - unsigned long dom = 0, bus = 0;
>>> - unsigned int slot = 0, func = 0;
>>> + char *str;
>>>
>>> if (dev->realized) {
>>> qdev_prop_set_after_realize(dev, name, errp);
>>> @@ -750,58 +742,11 @@ 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 != ':') {
>>> - 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) {
>>> - goto inval;
>>> - }
>>> - }
>>> - slot = val;
>>> -
>>> - if (*e != '.') {
>>> - goto inval;
>>> + if (!pci_host_device_address_parse(str, addr)) {
>>> + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
>>> }
>>> - p = e + 1;
>>> - val = strtoul(p, &e, 10);
>>> - if (e == p) {
>>> - goto inval;
>>> - }
>>> - func = val;
>>> -
>>> - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
>>> - goto inval;
>>> - }
>>> -
>>> - if (*e) {
>>> - goto inval;
>>> - }
>>> -
>>> - addr->domain = dom;
>>> - addr->bus = bus;
>>> - addr->slot = slot;
>>> - addr->function = func;
>>>
>>> g_free(str);
>>> - return;
>>> -
>>> -inval:
>>> - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
>>> - g_free(str);
>>> }
>>>
>>> PropertyInfo qdev_prop_pci_host_devaddr = {
>>> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
>>> index 20b0f59ba2..81c4f072f3 100644
>>> --- a/tests/test-cutils.c
>>> +++ b/tests/test-cutils.c
>>> @@ -28,6 +28,7 @@
>>> #include "qemu/osdep.h"
>>>
>>> #include "qemu/cutils.h"
>>> +#include "hw/pci/pci.h"
>>>
>>> static void test_parse_uint_null(void)
>>> {
>>> @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
>>> g_assert_cmpint(res, ==, 12345000);
>>> }
>>>
>>> +static void test_pci_parse_valid(void)
>>> +{
>>> + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
>>> + int i, res;
>>> + const struct {
>>> + const char *str;
>>> + PCIHostDeviceAddress addr;
>>> + } tests[] = {
>>> + { "1:2.3", { 0, 1, 2, 3 } },
>>> + { "4:1:2.3", { 4, 1, 2, 3 } },
>>> + { "a:a:a.7", { 10, 10, 10, 7 } },
>>> + };
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
>>> + res = pci_host_device_address_parse(tests[i].str, &addr);
>>> + g_assert_cmpint(res, ==, 0);
>>> +
>>> + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
>>> + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
>>> + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
>>> + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
>>> + }
>>> +}
>>> +
>>> +static void test_pci_parse_invalid(void)
>>> +{
>>> + int i, res;
>>> + const char *tests[] = {
>>> + "", "foo", "1:2.3 foo",
>>> + "a:a:a.a",
>>> + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
>>> + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
>>> + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
>>> + res = pci_host_device_address_parse(tests[i], NULL);
>>> + g_assert_cmpint(res, ==, -1);
>>> + }
>>> +}
>>> +
>>> int main(int argc, char **argv)
>>> {
>>> g_test_init(&argc, &argv, NULL);
>>> @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
>>> g_test_add_func("/cutils/strtosz/suffix-unit",
>>> test_qemu_strtosz_suffix_unit);
>>>
>>> + g_test_add_func("/cutils/pci-parse/valid",
>>> + test_pci_parse_valid);
>>> + g_test_add_func("/cutils/pci-parse/invalid",
>>> + test_pci_parse_invalid);
>>> +
>>> return g_test_run();
>>> }
>>> diff --git a/util/cutils.c b/util/cutils.c
>>> index 4fefcf3be3..fc71c85816 100644
>>> --- a/util/cutils.c
>>> +++ b/util/cutils.c
>>> @@ -29,6 +29,7 @@
>>> #include "qemu/sockets.h"
>>> #include "qemu/iov.h"
>>> #include "net/net.h"
>>> +#include "hw/pci/pci.h"
>>> #include "qemu/cutils.h"
>>>
>>> void strpadcpy(char *buf, int buf_size, const char *str, char pad)
>>> @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
>>>
>>> return ret;
>>> }
>>> +
>>> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
>>> *addr)
>>> +{
>>> + const char *p = str;
>>> + const char *e;
>>> + unsigned long val;
>>> + unsigned int dom = 0, bus = 0;
>>> + unsigned int slot = 0, func = 0;
>>> +
>>> + if (qemu_strtoul(p, &e, 16, &val) < 0
>>> + || e == p || *e != ':') {
>>> + return -1;
>>> + }
>>> + bus = val;
>>> +
>>> + p = e + 1;
>>> + if (qemu_strtoul(p, &e, 16, &val) < 0
>>> + || e == p) {
>>> + return -1;
>>> + }
>>> + if (*e == ':') {
>>> + dom = bus;
>>> + bus = val;
>>> + p = e + 1;
>>> + if (qemu_strtoul(p, &e, 16, &val) < 0
>>> + || e == p) {
>>> + return -1;
>>> + }
>>> + }
>>> + slot = val;
>>> +
>>> + if (*e != '.') {
>>> + return -1;
>>> + }
>>> + p = e + 1;
>>> + if (qemu_strtoul(p, &e, 10, &val) < 0
>>> + || e == p) {
>>> + return -1;
>>> + }
>>> + func = val;
>>> +
>>> + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
>>> + return -1;
>>> + }
>>> +
>>> + if (*e) {
>>> + return -1;
>>> + }
>>> +
>>> + if (addr) {
>>> + addr->domain = dom;
>>> + addr->bus = bus;
>>> + addr->slot = slot;
>>> + addr->function = func;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>>
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 11:30 ` Marcel Apfelbaum
@ 2017-02-16 12:08 ` Marc-André Lureau
0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2017-02-16 12:08 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: Marc-André Lureau, qemu-devel, mst
Hi
----- Original Message -----
> On 02/16/2017 01:19 PM, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> >> On 02/16/2017 12:58 PM, Marc-André Lureau wrote:
> >>> This will allow the function to be used outside of QOM properties.
> >>>
> >>
> >> Hi Mark-Andre,
> >>
> >> I am not sure cutil.h is the right header for the function, because is too
> >> PCI
> >> related for a utility library.
> >
> > It's in the pci.h header, implementation and test is put in
> > {test-,}cutils.c
> >
>
> sorry about the miss-match, I war referring to the implementation.
>
> > I can try to put it in pci/pci.c, but it will be harder to link to for
> > testing (note: I haven't tried). I could also put it in a seperate
> > pci-parse.c for example.
>
> no need for a new file, but hw/pci/pci_host.c should be a good place.
It pulls in pci/pci.c for pci_find_device() and friends, plus type registration etc.
I'll move it to a standalone pci/pci-util.c
>
> regarding the tests, an new pci test file would be welcomed.
>
> >
> >
> >> Also, maybe we can post the patch together with the code that uses the
> >> exposed function.
> >
> > I dropped the patch using it for now, but I thought it would still be nice
> > to have the test, doc and strtoul change.
> >
>
> I agree if Michael has nothing against this.
>
> Thanks,
> Marcel
>
> >>
> >> And thanks for the unit-tests, much appreciated!
> >
> > thanks
> >
> >> Marcel
> >>
> >>> Add tests and replace strtoul usage for qemu_strtoul while touching it.
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>> ---
> >>> v2:
> >>> - fixed incompatible type warning/error
> >>>
> >>> include/hw/pci/pci.h | 12 ++++++++++
> >>> hw/core/qdev-properties.c | 61
> >>> +++--------------------------------------------
> >>> tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
> >>> util/cutils.c | 59
> >>> +++++++++++++++++++++++++++++++++++++++++++++
> >>> 4 files changed, 120 insertions(+), 58 deletions(-)
> >>>
> >>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> >>> index cbc1fdfb5b..61718080ea 100644
> >>> --- a/include/hw/pci/pci.h
> >>> +++ b/include/hw/pci/pci.h
> >>> @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
> >>>
> >>> MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> >>>
> >>> +/**
> >>> + * pci_host_device_address_parse:
> >>> + * @str: string to parse
> >>> + * @addr: destination structure or NULL
> >>> + *
> >>> + * Parse [<domain>:]<bus>:<slot>.<func>
> >>> + * if <domain> is not supplied, it's assumed to be 0.
> >>> + *
> >>> + * Returns: -1 on failure, 0 on success.
> >>> + */
> >>> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> >>> *addr);
> >>> +
> >>> #endif
> >>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> >>> index 6ab4265eb4..5dd0774e88 100644
> >>> --- a/hw/core/qdev-properties.c
> >>> +++ b/hw/core/qdev-properties.c
> >>> @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj,
> >>> Visitor
> >>> *v, const char *name,
> >>> visit_type_str(v, name, &p, errp);
> >>> }
> >>>
> >>> -/*
> >>> - * Parse [<domain>:]<bus>:<slot>.<func>
> >>> - * if <domain> is not supplied, it's assumed to be 0.
> >>> - */
> >>> static void set_pci_host_devaddr(Object *obj, Visitor *v, const char
> >>> *name,
> >>> void *opaque, Error **errp)
> >>> {
> >>> @@ -733,11 +729,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);
> >>> Error *local_err = NULL;
> >>> - char *str, *p;
> >>> - char *e;
> >>> - unsigned long val;
> >>> - unsigned long dom = 0, bus = 0;
> >>> - unsigned int slot = 0, func = 0;
> >>> + char *str;
> >>>
> >>> if (dev->realized) {
> >>> qdev_prop_set_after_realize(dev, name, errp);
> >>> @@ -750,58 +742,11 @@ 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 != ':') {
> >>> - 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) {
> >>> - goto inval;
> >>> - }
> >>> - }
> >>> - slot = val;
> >>> -
> >>> - if (*e != '.') {
> >>> - goto inval;
> >>> + if (!pci_host_device_address_parse(str, addr)) {
> >>> + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> >>> }
> >>> - p = e + 1;
> >>> - val = strtoul(p, &e, 10);
> >>> - if (e == p) {
> >>> - goto inval;
> >>> - }
> >>> - func = val;
> >>> -
> >>> - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> >>> - goto inval;
> >>> - }
> >>> -
> >>> - if (*e) {
> >>> - goto inval;
> >>> - }
> >>> -
> >>> - addr->domain = dom;
> >>> - addr->bus = bus;
> >>> - addr->slot = slot;
> >>> - addr->function = func;
> >>>
> >>> g_free(str);
> >>> - return;
> >>> -
> >>> -inval:
> >>> - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> >>> - g_free(str);
> >>> }
> >>>
> >>> PropertyInfo qdev_prop_pci_host_devaddr = {
> >>> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> >>> index 20b0f59ba2..81c4f072f3 100644
> >>> --- a/tests/test-cutils.c
> >>> +++ b/tests/test-cutils.c
> >>> @@ -28,6 +28,7 @@
> >>> #include "qemu/osdep.h"
> >>>
> >>> #include "qemu/cutils.h"
> >>> +#include "hw/pci/pci.h"
> >>>
> >>> static void test_parse_uint_null(void)
> >>> {
> >>> @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
> >>> g_assert_cmpint(res, ==, 12345000);
> >>> }
> >>>
> >>> +static void test_pci_parse_valid(void)
> >>> +{
> >>> + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
> >>> + int i, res;
> >>> + const struct {
> >>> + const char *str;
> >>> + PCIHostDeviceAddress addr;
> >>> + } tests[] = {
> >>> + { "1:2.3", { 0, 1, 2, 3 } },
> >>> + { "4:1:2.3", { 4, 1, 2, 3 } },
> >>> + { "a:a:a.7", { 10, 10, 10, 7 } },
> >>> + };
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> >>> + res = pci_host_device_address_parse(tests[i].str, &addr);
> >>> + g_assert_cmpint(res, ==, 0);
> >>> +
> >>> + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
> >>> + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
> >>> + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
> >>> + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
> >>> + }
> >>> +}
> >>> +
> >>> +static void test_pci_parse_invalid(void)
> >>> +{
> >>> + int i, res;
> >>> + const char *tests[] = {
> >>> + "", "foo", "1:2.3 foo",
> >>> + "a:a:a.a",
> >>> + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
> >>> + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
> >>> + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
> >>> +
> >>> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> >>> + res = pci_host_device_address_parse(tests[i], NULL);
> >>> + g_assert_cmpint(res, ==, -1);
> >>> + }
> >>> +}
> >>> +
> >>> int main(int argc, char **argv)
> >>> {
> >>> g_test_init(&argc, &argv, NULL);
> >>> @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
> >>> g_test_add_func("/cutils/strtosz/suffix-unit",
> >>> test_qemu_strtosz_suffix_unit);
> >>>
> >>> + g_test_add_func("/cutils/pci-parse/valid",
> >>> + test_pci_parse_valid);
> >>> + g_test_add_func("/cutils/pci-parse/invalid",
> >>> + test_pci_parse_invalid);
> >>> +
> >>> return g_test_run();
> >>> }
> >>> diff --git a/util/cutils.c b/util/cutils.c
> >>> index 4fefcf3be3..fc71c85816 100644
> >>> --- a/util/cutils.c
> >>> +++ b/util/cutils.c
> >>> @@ -29,6 +29,7 @@
> >>> #include "qemu/sockets.h"
> >>> #include "qemu/iov.h"
> >>> #include "net/net.h"
> >>> +#include "hw/pci/pci.h"
> >>> #include "qemu/cutils.h"
> >>>
> >>> void strpadcpy(char *buf, int buf_size, const char *str, char pad)
> >>> @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
> >>>
> >>> return ret;
> >>> }
> >>> +
> >>> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> >>> *addr)
> >>> +{
> >>> + const char *p = str;
> >>> + const char *e;
> >>> + unsigned long val;
> >>> + unsigned int dom = 0, bus = 0;
> >>> + unsigned int slot = 0, func = 0;
> >>> +
> >>> + if (qemu_strtoul(p, &e, 16, &val) < 0
> >>> + || e == p || *e != ':') {
> >>> + return -1;
> >>> + }
> >>> + bus = val;
> >>> +
> >>> + p = e + 1;
> >>> + if (qemu_strtoul(p, &e, 16, &val) < 0
> >>> + || e == p) {
> >>> + return -1;
> >>> + }
> >>> + if (*e == ':') {
> >>> + dom = bus;
> >>> + bus = val;
> >>> + p = e + 1;
> >>> + if (qemu_strtoul(p, &e, 16, &val) < 0
> >>> + || e == p) {
> >>> + return -1;
> >>> + }
> >>> + }
> >>> + slot = val;
> >>> +
> >>> + if (*e != '.') {
> >>> + return -1;
> >>> + }
> >>> + p = e + 1;
> >>> + if (qemu_strtoul(p, &e, 10, &val) < 0
> >>> + || e == p) {
> >>> + return -1;
> >>> + }
> >>> + func = val;
> >>> +
> >>> + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> >>> + return -1;
> >>> + }
> >>> +
> >>> + if (*e) {
> >>> + return -1;
> >>> + }
> >>> +
> >>> + if (addr) {
> >>> + addr->domain = dom;
> >>> + addr->bus = bus;
> >>> + addr->slot = slot;
> >>> + addr->function = func;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>>
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 10:58 [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse() Marc-André Lureau
2017-02-16 11:13 ` Marcel Apfelbaum
@ 2017-02-16 14:37 ` Michael S. Tsirkin
2017-02-16 15:07 ` Marc-André Lureau
1 sibling, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-02-16 14:37 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, marcel
On Thu, Feb 16, 2017 at 02:58:27PM +0400, Marc-André Lureau wrote:
> This will allow the function to be used outside of QOM properties.
>
> Add tests and replace strtoul usage for qemu_strtoul while touching it.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Sorry, I don't want to start maintaining it. So pls do not move it
to hw/pci. Find out who wants to maintain this and add it there :)
> ---
> v2:
> - fixed incompatible type warning/error
>
> include/hw/pci/pci.h | 12 ++++++++++
> hw/core/qdev-properties.c | 61 +++--------------------------------------------
> tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
> util/cutils.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 120 insertions(+), 58 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index cbc1fdfb5b..61718080ea 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
>
> MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
>
> +/**
> + * pci_host_device_address_parse:
> + * @str: string to parse
> + * @addr: destination structure or NULL
> + *
> + * Parse [<domain>:]<bus>:<slot>.<func>
> + * if <domain> is not supplied, it's assumed to be 0.
> + *
> + * Returns: -1 on failure, 0 on success.
> + */
> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress *addr);
> +
> #endif
Used to be a static function, now it's an interface, I'm not sure it's a win.
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 6ab4265eb4..5dd0774e88 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> visit_type_str(v, name, &p, errp);
> }
>
> -/*
> - * Parse [<domain>:]<bus>:<slot>.<func>
> - * if <domain> is not supplied, it's assumed to be 0.
> - */
> static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> @@ -733,11 +729,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);
> Error *local_err = NULL;
> - char *str, *p;
> - char *e;
> - unsigned long val;
> - unsigned long dom = 0, bus = 0;
> - unsigned int slot = 0, func = 0;
> + char *str;
>
> if (dev->realized) {
> qdev_prop_set_after_realize(dev, name, errp);
> @@ -750,58 +742,11 @@ 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 != ':') {
> - 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) {
> - goto inval;
> - }
> - }
> - slot = val;
> -
> - if (*e != '.') {
> - goto inval;
> + if (!pci_host_device_address_parse(str, addr)) {
> + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> }
> - p = e + 1;
> - val = strtoul(p, &e, 10);
> - if (e == p) {
> - goto inval;
> - }
> - func = val;
> -
> - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> - goto inval;
> - }
> -
> - if (*e) {
> - goto inval;
> - }
> -
> - addr->domain = dom;
> - addr->bus = bus;
> - addr->slot = slot;
> - addr->function = func;
>
> g_free(str);
> - return;
> -
> -inval:
> - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> - g_free(str);
> }
>
> PropertyInfo qdev_prop_pci_host_devaddr = {
> diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> index 20b0f59ba2..81c4f072f3 100644
> --- a/tests/test-cutils.c
> +++ b/tests/test-cutils.c
> @@ -28,6 +28,7 @@
> #include "qemu/osdep.h"
>
> #include "qemu/cutils.h"
> +#include "hw/pci/pci.h"
>
> static void test_parse_uint_null(void)
> {
> @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
> g_assert_cmpint(res, ==, 12345000);
> }
>
> +static void test_pci_parse_valid(void)
> +{
> + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
> + int i, res;
> + const struct {
> + const char *str;
> + PCIHostDeviceAddress addr;
> + } tests[] = {
> + { "1:2.3", { 0, 1, 2, 3 } },
> + { "4:1:2.3", { 4, 1, 2, 3 } },
> + { "a:a:a.7", { 10, 10, 10, 7 } },
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> + res = pci_host_device_address_parse(tests[i].str, &addr);
> + g_assert_cmpint(res, ==, 0);
> +
> + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
> + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
> + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
> + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
> + }
> +}
> +
> +static void test_pci_parse_invalid(void)
> +{
> + int i, res;
> + const char *tests[] = {
> + "", "foo", "1:2.3 foo",
> + "a:a:a.a",
> + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
> + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
> + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
> +
> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> + res = pci_host_device_address_parse(tests[i], NULL);
> + g_assert_cmpint(res, ==, -1);
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
Pls find a way to test is through a property.
> @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
> g_test_add_func("/cutils/strtosz/suffix-unit",
> test_qemu_strtosz_suffix_unit);
>
> + g_test_add_func("/cutils/pci-parse/valid",
> + test_pci_parse_valid);
> + g_test_add_func("/cutils/pci-parse/invalid",
> + test_pci_parse_invalid);
> +
> return g_test_run();
> }
> diff --git a/util/cutils.c b/util/cutils.c
> index 4fefcf3be3..fc71c85816 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -29,6 +29,7 @@
> #include "qemu/sockets.h"
> #include "qemu/iov.h"
> #include "net/net.h"
> +#include "hw/pci/pci.h"
> #include "qemu/cutils.h"
>
> void strpadcpy(char *buf, int buf_size, const char *str, char pad)
> @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
>
> return ret;
> }
> +
> +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress *addr)
> +{
> + const char *p = str;
> + const char *e;
> + unsigned long val;
> + unsigned int dom = 0, bus = 0;
> + unsigned int slot = 0, func = 0;
> +
> + if (qemu_strtoul(p, &e, 16, &val) < 0
> + || e == p || *e != ':') {
> + return -1;
> + }
> + bus = val;
> +
> + p = e + 1;
> + if (qemu_strtoul(p, &e, 16, &val) < 0
> + || e == p) {
> + return -1;
> + }
> + if (*e == ':') {
> + dom = bus;
> + bus = val;
> + p = e + 1;
> + if (qemu_strtoul(p, &e, 16, &val) < 0
> + || e == p) {
> + return -1;
> + }
> + }
> + slot = val;
> +
> + if (*e != '.') {
> + return -1;
> + }
> + p = e + 1;
> + if (qemu_strtoul(p, &e, 10, &val) < 0
> + || e == p) {
> + return -1;
> + }
> + func = val;
> +
> + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> + return -1;
> + }
> +
> + if (*e) {
> + return -1;
> + }
> +
> + if (addr) {
> + addr->domain = dom;
> + addr->bus = bus;
> + addr->slot = slot;
> + addr->function = func;
> + }
> +
> + return 0;
> +}
> --
> 2.11.0.295.gd7dffce1c.dirty
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 14:37 ` Michael S. Tsirkin
@ 2017-02-16 15:07 ` Marc-André Lureau
2017-02-16 15:29 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2017-02-16 15:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Marc-André Lureau, qemu-devel, marcel
Hi
----- Original Message -----
> On Thu, Feb 16, 2017 at 02:58:27PM +0400, Marc-André Lureau wrote:
> > This will allow the function to be used outside of QOM properties.
> >
> > Add tests and replace strtoul usage for qemu_strtoul while touching it.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Sorry, I don't want to start maintaining it. So pls do not move it
> to hw/pci. Find out who wants to maintain this and add it there :)
Well, if you are PCI maintainer, I guess it falls into you anyway ;)
>
> > ---
> > v2:
> > - fixed incompatible type warning/error
> >
> > include/hw/pci/pci.h | 12 ++++++++++
> > hw/core/qdev-properties.c | 61
> > +++--------------------------------------------
> > tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
> > util/cutils.c | 59
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 120 insertions(+), 58 deletions(-)
> >
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index cbc1fdfb5b..61718080ea 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
> >
> > MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> >
> > +/**
> > + * pci_host_device_address_parse:
> > + * @str: string to parse
> > + * @addr: destination structure or NULL
> > + *
> > + * Parse [<domain>:]<bus>:<slot>.<func>
> > + * if <domain> is not supplied, it's assumed to be 0.
> > + *
> > + * Returns: -1 on failure, 0 on success.
> > + */
> > +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> > *addr);
> > +
> > #endif
>
> Used to be a static function, now it's an interface, I'm not sure it's a win.
It's an internal interface, with a single user and a test. Not a big deal.
>
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index 6ab4265eb4..5dd0774e88 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor
> > *v, const char *name,
> > visit_type_str(v, name, &p, errp);
> > }
> >
> > -/*
> > - * Parse [<domain>:]<bus>:<slot>.<func>
> > - * if <domain> is not supplied, it's assumed to be 0.
> > - */
> > static void set_pci_host_devaddr(Object *obj, Visitor *v, const char
> > *name,
> > void *opaque, Error **errp)
> > {
> > @@ -733,11 +729,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);
> > Error *local_err = NULL;
> > - char *str, *p;
> > - char *e;
> > - unsigned long val;
> > - unsigned long dom = 0, bus = 0;
> > - unsigned int slot = 0, func = 0;
> > + char *str;
> >
> > if (dev->realized) {
> > qdev_prop_set_after_realize(dev, name, errp);
> > @@ -750,58 +742,11 @@ 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 != ':') {
> > - 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) {
> > - goto inval;
> > - }
> > - }
> > - slot = val;
> > -
> > - if (*e != '.') {
> > - goto inval;
> > + if (!pci_host_device_address_parse(str, addr)) {
> > + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > }
> > - p = e + 1;
> > - val = strtoul(p, &e, 10);
> > - if (e == p) {
> > - goto inval;
> > - }
> > - func = val;
> > -
> > - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> > - goto inval;
> > - }
> > -
> > - if (*e) {
> > - goto inval;
> > - }
> > -
> > - addr->domain = dom;
> > - addr->bus = bus;
> > - addr->slot = slot;
> > - addr->function = func;
> >
> > g_free(str);
> > - return;
> > -
> > -inval:
> > - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > - g_free(str);
> > }
> >
> > PropertyInfo qdev_prop_pci_host_devaddr = {
> > diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> > index 20b0f59ba2..81c4f072f3 100644
> > --- a/tests/test-cutils.c
> > +++ b/tests/test-cutils.c
> > @@ -28,6 +28,7 @@
> > #include "qemu/osdep.h"
> >
> > #include "qemu/cutils.h"
> > +#include "hw/pci/pci.h"
> >
> > static void test_parse_uint_null(void)
> > {
> > @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
> > g_assert_cmpint(res, ==, 12345000);
> > }
> >
> > +static void test_pci_parse_valid(void)
> > +{
> > + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
> > + int i, res;
> > + const struct {
> > + const char *str;
> > + PCIHostDeviceAddress addr;
> > + } tests[] = {
> > + { "1:2.3", { 0, 1, 2, 3 } },
> > + { "4:1:2.3", { 4, 1, 2, 3 } },
> > + { "a:a:a.7", { 10, 10, 10, 7 } },
> > + };
> > +
> > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > + res = pci_host_device_address_parse(tests[i].str, &addr);
> > + g_assert_cmpint(res, ==, 0);
> > +
> > + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
> > + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
> > + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
> > + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
> > + }
> > +}
> > +
> > +static void test_pci_parse_invalid(void)
> > +{
> > + int i, res;
> > + const char *tests[] = {
> > + "", "foo", "1:2.3 foo",
> > + "a:a:a.a",
> > + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
> > + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
> > + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
> > +
> > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > + res = pci_host_device_address_parse(tests[i], NULL);
> > + g_assert_cmpint(res, ==, -1);
> > + }
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > g_test_init(&argc, &argv, NULL);
>
>
> Pls find a way to test is through a property.
Probably doable, though it's less of a unit test then. I'll look into doing it.
>
> > @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
> > g_test_add_func("/cutils/strtosz/suffix-unit",
> > test_qemu_strtosz_suffix_unit);
> >
> > + g_test_add_func("/cutils/pci-parse/valid",
> > + test_pci_parse_valid);
> > + g_test_add_func("/cutils/pci-parse/invalid",
> > + test_pci_parse_invalid);
> > +
> > return g_test_run();
> > }
> > diff --git a/util/cutils.c b/util/cutils.c
> > index 4fefcf3be3..fc71c85816 100644
> > --- a/util/cutils.c
> > +++ b/util/cutils.c
> > @@ -29,6 +29,7 @@
> > #include "qemu/sockets.h"
> > #include "qemu/iov.h"
> > #include "net/net.h"
> > +#include "hw/pci/pci.h"
> > #include "qemu/cutils.h"
> >
> > void strpadcpy(char *buf, int buf_size, const char *str, char pad)
> > @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
> >
> > return ret;
> > }
> > +
> > +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> > *addr)
> > +{
> > + const char *p = str;
> > + const char *e;
> > + unsigned long val;
> > + unsigned int dom = 0, bus = 0;
> > + unsigned int slot = 0, func = 0;
> > +
> > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > + || e == p || *e != ':') {
> > + return -1;
> > + }
> > + bus = val;
> > +
> > + p = e + 1;
> > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > + || e == p) {
> > + return -1;
> > + }
> > + if (*e == ':') {
> > + dom = bus;
> > + bus = val;
> > + p = e + 1;
> > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > + || e == p) {
> > + return -1;
> > + }
> > + }
> > + slot = val;
> > +
> > + if (*e != '.') {
> > + return -1;
> > + }
> > + p = e + 1;
> > + if (qemu_strtoul(p, &e, 10, &val) < 0
> > + || e == p) {
> > + return -1;
> > + }
> > + func = val;
> > +
> > + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> > + return -1;
> > + }
> > +
> > + if (*e) {
> > + return -1;
> > + }
> > +
> > + if (addr) {
> > + addr->domain = dom;
> > + addr->bus = bus;
> > + addr->slot = slot;
> > + addr->function = func;
> > + }
> > +
> > + return 0;
> > +}
> > --
> > 2.11.0.295.gd7dffce1c.dirty
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse()
2017-02-16 15:07 ` Marc-André Lureau
@ 2017-02-16 15:29 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-02-16 15:29 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Marc-André Lureau, qemu-devel, marcel
On Thu, Feb 16, 2017 at 10:07:50AM -0500, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
> > On Thu, Feb 16, 2017 at 02:58:27PM +0400, Marc-André Lureau wrote:
> > > This will allow the function to be used outside of QOM properties.
> > >
> > > Add tests and replace strtoul usage for qemu_strtoul while touching it.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Sorry, I don't want to start maintaining it. So pls do not move it
> > to hw/pci. Find out who wants to maintain this and add it there :)
>
>
> Well, if you are PCI maintainer, I guess it falls into you anyway ;)
I don't get to maintain all pci devices. This parsing code is part of
QMP/HMP interfaces with pass-through devices. Not part of pci emulation
which is what I maintain.
> >
> > > ---
> > > v2:
> > > - fixed incompatible type warning/error
> > >
> > > include/hw/pci/pci.h | 12 ++++++++++
> > > hw/core/qdev-properties.c | 61
> > > +++--------------------------------------------
> > > tests/test-cutils.c | 46 +++++++++++++++++++++++++++++++++++
> > > util/cutils.c | 59
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 120 insertions(+), 58 deletions(-)
> > >
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index cbc1fdfb5b..61718080ea 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -815,4 +815,16 @@ extern const VMStateDescription vmstate_pci_device;
> > >
> > > MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
> > >
> > > +/**
> > > + * pci_host_device_address_parse:
> > > + * @str: string to parse
> > > + * @addr: destination structure or NULL
> > > + *
> > > + * Parse [<domain>:]<bus>:<slot>.<func>
> > > + * if <domain> is not supplied, it's assumed to be 0.
> > > + *
> > > + * Returns: -1 on failure, 0 on success.
> > > + */
> > > +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> > > *addr);
> > > +
> > > #endif
> >
> > Used to be a static function, now it's an interface, I'm not sure it's a win.
>
> It's an internal interface, with a single user and a test. Not a big deal.
>
> >
> > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > index 6ab4265eb4..5dd0774e88 100644
> > > --- a/hw/core/qdev-properties.c
> > > +++ b/hw/core/qdev-properties.c
> > > @@ -722,10 +722,6 @@ static void get_pci_host_devaddr(Object *obj, Visitor
> > > *v, const char *name,
> > > visit_type_str(v, name, &p, errp);
> > > }
> > >
> > > -/*
> > > - * Parse [<domain>:]<bus>:<slot>.<func>
> > > - * if <domain> is not supplied, it's assumed to be 0.
> > > - */
> > > static void set_pci_host_devaddr(Object *obj, Visitor *v, const char
> > > *name,
> > > void *opaque, Error **errp)
> > > {
> > > @@ -733,11 +729,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);
> > > Error *local_err = NULL;
> > > - char *str, *p;
> > > - char *e;
> > > - unsigned long val;
> > > - unsigned long dom = 0, bus = 0;
> > > - unsigned int slot = 0, func = 0;
> > > + char *str;
> > >
> > > if (dev->realized) {
> > > qdev_prop_set_after_realize(dev, name, errp);
> > > @@ -750,58 +742,11 @@ 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 != ':') {
> > > - 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) {
> > > - goto inval;
> > > - }
> > > - }
> > > - slot = val;
> > > -
> > > - if (*e != '.') {
> > > - goto inval;
> > > + if (!pci_host_device_address_parse(str, addr)) {
> > > + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > > }
> > > - p = e + 1;
> > > - val = strtoul(p, &e, 10);
> > > - if (e == p) {
> > > - goto inval;
> > > - }
> > > - func = val;
> > > -
> > > - if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> > > - goto inval;
> > > - }
> > > -
> > > - if (*e) {
> > > - goto inval;
> > > - }
> > > -
> > > - addr->domain = dom;
> > > - addr->bus = bus;
> > > - addr->slot = slot;
> > > - addr->function = func;
> > >
> > > g_free(str);
> > > - return;
> > > -
> > > -inval:
> > > - error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > > - g_free(str);
> > > }
> > >
> > > PropertyInfo qdev_prop_pci_host_devaddr = {
> > > diff --git a/tests/test-cutils.c b/tests/test-cutils.c
> > > index 20b0f59ba2..81c4f072f3 100644
> > > --- a/tests/test-cutils.c
> > > +++ b/tests/test-cutils.c
> > > @@ -28,6 +28,7 @@
> > > #include "qemu/osdep.h"
> > >
> > > #include "qemu/cutils.h"
> > > +#include "hw/pci/pci.h"
> > >
> > > static void test_parse_uint_null(void)
> > > {
> > > @@ -1437,6 +1438,46 @@ static void test_qemu_strtosz_suffix_unit(void)
> > > g_assert_cmpint(res, ==, 12345000);
> > > }
> > >
> > > +static void test_pci_parse_valid(void)
> > > +{
> > > + PCIHostDeviceAddress addr = { 0xff, 0xff, 0xff, 0xff };
> > > + int i, res;
> > > + const struct {
> > > + const char *str;
> > > + PCIHostDeviceAddress addr;
> > > + } tests[] = {
> > > + { "1:2.3", { 0, 1, 2, 3 } },
> > > + { "4:1:2.3", { 4, 1, 2, 3 } },
> > > + { "a:a:a.7", { 10, 10, 10, 7 } },
> > > + };
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > > + res = pci_host_device_address_parse(tests[i].str, &addr);
> > > + g_assert_cmpint(res, ==, 0);
> > > +
> > > + g_assert_cmpint(addr.domain, ==, tests[i].addr.domain);
> > > + g_assert_cmpint(addr.bus, ==, tests[i].addr.bus);
> > > + g_assert_cmpint(addr.slot, ==, tests[i].addr.slot);
> > > + g_assert_cmpint(addr.function, ==, tests[i].addr.function);
> > > + }
> > > +}
> > > +
> > > +static void test_pci_parse_invalid(void)
> > > +{
> > > + int i, res;
> > > + const char *tests[] = {
> > > + "", "foo", "1:2.3 foo",
> > > + "a:a:a.a",
> > > + "1", "1:", "1:2", "1:2:", "1:2:3", ":1:2:3.",
> > > + "-1:1:2.3", "-1:2.3", "1:-1.3", "1:2.-3",
> > > + "0x10000:1:1.1", "0x100:1.1", "1:0x20.1", "1:1.8" };
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> > > + res = pci_host_device_address_parse(tests[i], NULL);
> > > + g_assert_cmpint(res, ==, -1);
> > > + }
> > > +}
> > > +
> > > int main(int argc, char **argv)
> > > {
> > > g_test_init(&argc, &argv, NULL);
> >
> >
> > Pls find a way to test is through a property.
>
> Probably doable, though it's less of a unit test then. I'll look into doing it.
>
> >
> > > @@ -1598,5 +1639,10 @@ int main(int argc, char **argv)
> > > g_test_add_func("/cutils/strtosz/suffix-unit",
> > > test_qemu_strtosz_suffix_unit);
> > >
> > > + g_test_add_func("/cutils/pci-parse/valid",
> > > + test_pci_parse_valid);
> > > + g_test_add_func("/cutils/pci-parse/invalid",
> > > + test_pci_parse_invalid);
> > > +
> > > return g_test_run();
> > > }
> > > diff --git a/util/cutils.c b/util/cutils.c
> > > index 4fefcf3be3..fc71c85816 100644
> > > --- a/util/cutils.c
> > > +++ b/util/cutils.c
> > > @@ -29,6 +29,7 @@
> > > #include "qemu/sockets.h"
> > > #include "qemu/iov.h"
> > > #include "net/net.h"
> > > +#include "hw/pci/pci.h"
> > > #include "qemu/cutils.h"
> > >
> > > void strpadcpy(char *buf, int buf_size, const char *str, char pad)
> > > @@ -594,3 +595,61 @@ const char *qemu_ether_ntoa(const MACAddr *mac)
> > >
> > > return ret;
> > > }
> > > +
> > > +int pci_host_device_address_parse(const char *str, PCIHostDeviceAddress
> > > *addr)
> > > +{
> > > + const char *p = str;
> > > + const char *e;
> > > + unsigned long val;
> > > + unsigned int dom = 0, bus = 0;
> > > + unsigned int slot = 0, func = 0;
> > > +
> > > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > > + || e == p || *e != ':') {
> > > + return -1;
> > > + }
> > > + bus = val;
> > > +
> > > + p = e + 1;
> > > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > > + || e == p) {
> > > + return -1;
> > > + }
> > > + if (*e == ':') {
> > > + dom = bus;
> > > + bus = val;
> > > + p = e + 1;
> > > + if (qemu_strtoul(p, &e, 16, &val) < 0
> > > + || e == p) {
> > > + return -1;
> > > + }
> > > + }
> > > + slot = val;
> > > +
> > > + if (*e != '.') {
> > > + return -1;
> > > + }
> > > + p = e + 1;
> > > + if (qemu_strtoul(p, &e, 10, &val) < 0
> > > + || e == p) {
> > > + return -1;
> > > + }
> > > + func = val;
> > > +
> > > + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
> > > + return -1;
> > > + }
> > > +
> > > + if (*e) {
> > > + return -1;
> > > + }
> > > +
> > > + if (addr) {
> > > + addr->domain = dom;
> > > + addr->bus = bus;
> > > + addr->slot = slot;
> > > + addr->function = func;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > --
> > > 2.11.0.295.gd7dffce1c.dirty
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-16 15:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 10:58 [Qemu-devel] [PATCHv2] cutils: factor out pci_host_device_address_parse() Marc-André Lureau
2017-02-16 11:13 ` Marcel Apfelbaum
2017-02-16 11:19 ` Marc-André Lureau
2017-02-16 11:30 ` Marcel Apfelbaum
2017-02-16 12:08 ` Marc-André Lureau
2017-02-16 14:37 ` Michael S. Tsirkin
2017-02-16 15:07 ` Marc-André Lureau
2017-02-16 15:29 ` Michael S. Tsirkin
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.