Skip to contentSkip to conversation

pci devaddr parsing cleanups

PATCH In review Michael S. Tsirkin 4 participants 13 messages Feb 16, 2012 – Feb 19, 2012

lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

cover pci devaddr parsing cleanups

Michael S. Tsirkin (2):
pci: don't export an internal function
pci: rewrite devaddr parsing

 hw/pci.c |   83 +++++++++++++++++++++++++++++--------------------------------
 hw/pci.h |    2 -
 2 files changed, 39 insertions(+), 46 deletions(-)
Signature · 1 line
1.7.9.111.gf3fb0
author
#1 lore raw

PATCH 1/2 pci: don't export an internal function

Make an internal function, pci_parse_devaddr,
static.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/pci.c |    2 +-
 hw/pci.h |    2 --
 2 files changed, 1 insertions(+), 3 deletions(-)
hw/pci.c +1 -1
index f580bb5..5827c0e 100644
@@ -478,7 +478,7 @@ static void pci_set_default_subsystem_id(PCIDevice *pci_dev)
  * Parse [[<domain>:]<bus>:]<slot>, return -1 on error if funcp == NULL
  *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
  */
-int pci_parse_devaddr(const char *addr, int *domp, int *busp,
+static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
                       unsigned int *slotp, unsigned int *funcp)
 {
     const char *p;
hw/pci.h +0 -2
index 355c905..fd40b5e 100644
@@ -311,8 +311,6 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
 int pci_qdev_find_device(const char *id, PCIDevice **pdev);
 PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
 
-int pci_parse_devaddr(const char *addr, int *domp, int *busp,
-                      unsigned int *slotp, unsigned int *funcp);
 int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
                      unsigned *slotp);
Signature · 1 line
1.7.9.111.gf3fb0
author
#2 lore raw

PATCH 2/2 pci: rewrite devaddr parsing

In reply to Michael S. Tsirkin

Use scanf instead of manual string scanning.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/pci.c |   81 +++++++++++++++++++++++++++++---------------------------------
 1 files changed, 38 insertions(+), 43 deletions(-)
hw/pci.c +38 -43
index 5827c0e..a8c0b69 100644
@@ -479,61 +479,56 @@ static void pci_set_default_subsystem_id(PCIDevice *pci_dev)
  *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
  */
 static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
-                      unsigned int *slotp, unsigned int *funcp)
-{
-    const char *p;
-    char *e;
-    unsigned long val;
-    unsigned long dom = 0, bus = 0;
-    unsigned int slot = 0;
-    unsigned int func = 0;
-
-    p = addr;
-    val = strtoul(p, &e, 16);
-    if (e == p)
-	return -1;
-    if (*e == ':') {
-	bus = val;
-	p = e + 1;
-	val = strtoul(p, &e, 16);
-	if (e == p)
-	    return -1;
-	if (*e == ':') {
-	    dom = bus;
-	    bus = val;
-	    p = e + 1;
-	    val = strtoul(p, &e, 16);
-	    if (e == p)
-		return -1;
-	}
-    }
-
-    slot = val;
-
-    if (funcp != NULL) {
-        if (*e != '.')
-            return -1;
+                             unsigned int *slotp, unsigned int *funcp)
+{
+    unsigned dom, bus, slot, func;
+    int n = -1;
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
+    if (n == -1) {
+        dom = 0;
+        sscanf(addr, "%x:%x%n", &bus, &slot, &n);
+        if (n == -1) {
+            bus = 0;
+            sscanf(addr, "%x%n", &slot, &n);
+            if (n == -1) {
+                return -1;
+            }
+        }
+    }
 
-        p = e + 1;
-        val = strtoul(p, &e, 16);
-        if (e == p)
+    /* Parse the optional .func */
+    addr += n;
+    if (!*addr) {
+        func = 0;
+    } else {
+        sscanf(addr, ".%x%n", &func, &n);
+        if (n == -1) {
             return -1;
+        }
+        addr += n;
+    }
 
-        func = val;
+    /* Anything left? Malformed input. */
+    if (*addr) {
+        return -1;
     }
 
-    /* if funcp == NULL func is 0 */
-    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
-	return -1;
+    if (funcp == NULL) {
+        func = 0;
+    }
 
-    if (*e)
+    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
 	return -1;
+    }
 
     *domp = dom;
     *busp = bus;
     *slotp = slot;
-    if (funcp != NULL)
+    if (funcp != NULL) {
         *funcp = func;
+    }
     return 0;
 }
Signature · 1 line
1.7.9.111.gf3fb0
#3 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 46 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/pci.c |   81 +++++++++++++++++++++++++++++---------------------------------
 1 files changed, 38 insertions(+), 43 deletions(-)
hw/pci.c +0 -31
index 5827c0e..a8c0b69 100644
@@ -479,61 +479,56 @@ static void pci_set_default_subsystem_id(PCIDevice *pci_dev)
  *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
  */
 static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
-                      unsigned int *slotp, unsigned int *funcp)
-{
-    const char *p;
-    char *e;
-    unsigned long val;
-    unsigned long dom = 0, bus = 0;
-    unsigned int slot = 0;
-    unsigned int func = 0;
-
-    p = addr;
-    val = strtoul(p, &e, 16);
-    if (e == p)
-	return -1;
-    if (*e == ':') {
-	bus = val;
-	p = e + 1;
-	val = strtoul(p, &e, 16);
-	if (e == p)
-	    return -1;
-	if (*e == ':') {
-	    dom = bus;
-	    bus = val;
-	    p = e + 1;
-	    val = strtoul(p, &e, 16);
-	    if (e == p)
-		return -1;
-	}
-    }
-
-    slot = val;
-

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 10 lines
hw/pci.c +7 -3
-    if (funcp != NULL) {
-        if (*e != '.')
-            return -1;
+                             unsigned int *slotp, unsigned int *funcp)
+{
+    unsigned dom, bus, slot, func;
+    int n = -1;
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Quoted 56 lines
hw/pci.c +31 -9
+    if (n == -1) {
+        dom = 0;
+        sscanf(addr, "%x:%x%n", &bus, &slot, &n);
+        if (n == -1) {
+            bus = 0;
+            sscanf(addr, "%x%n", &slot, &n);
+            if (n == -1) {
+                return -1;
+            }
+        }
+    }
 
-        p = e + 1;
-        val = strtoul(p, &e, 16);
-        if (e == p)
+    /* Parse the optional .func */
+    addr += n;
+    if (!*addr) {
+        func = 0;
+    } else {
+        sscanf(addr, ".%x%n", &func, &n);
+        if (n == -1) {
             return -1;
+        }
+        addr += n;
+    }
 
-        func = val;
+    /* Anything left? Malformed input. */
+    if (*addr) {
+        return -1;
     }
 
-    /* if funcp == NULL func is 0 */
-    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
-	return -1;
+    if (funcp == NULL) {
+        func = 0;
+    }
 
-    if (*e)
+    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
 	return -1;
+    }
 
     *domp = dom;
     *busp = bus;
     *slotp = slot;
-    if (funcp != NULL)
+    if (funcp != NULL) {
         *funcp = func;
+    }
     return 0;
 }
Signature · 1 line
mailto:[email protected]
#4 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 5 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

hw/pci.c +3 -0
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow. If addr contains "100000000000000:0:0", there's no telling whether it will be diagnosed as a parse error, or silently accepted and truncated, in which case, there's no telling what dom will contain.

I cringe any time I see someone using scanf to parse numbers from arbitrary user input; I barely tolerate it for parsing things generated by the kernel, but even there, I won't ever use scanf myself. Same goes for atoi. _Only_ strtol and friends can robustly parse arbitrary input into integers.

Signature · 2 lines
Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#5 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 16 lines · Eric Blake
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 5 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

hw/pci.c +3 -0
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow. If addr contains "100000000000000:0:0", there's no telling whether it will be diagnosed as a parse error, or silently accepted and truncated, in which case, there's no telling what dom will contain.

Quoted 5 lines

I cringe any time I see someone using scanf to parse numbers from arbitrary user input; I barely tolerate it for parsing things generated by the kernel, but even there, I won't ever use scanf myself. Same goes for atoi. _Only_ strtol and friends can robustly parse arbitrary input into integers.

To whomever wanted to apply this - don't.

Signature · 1 line
mailto:[email protected]
author
#6 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

In reply to Eric Blake
Quoted 16 lines · Eric Blake
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 5 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

hw/pci.c +3 -0
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow. If addr contains "100000000000000:0:0", there's no telling whether it will be diagnosed as a parse error, or silently accepted and truncated, in which case, there's no telling what dom will contain.

Quoted 6 lines

I cringe any time I see someone using scanf to parse numbers from arbitrary user input; I barely tolerate it for parsing things generated by the kernel, but even there, I won't ever use scanf myself. Same goes
for atoi. _Only_ strtol and friends can robustly parse arbitrary input into integers.

Seems easy to fix: I'll just set maximum field width of 8. Any other issues?

Quoted 4 lines
Signature · 2 lines
Eric Blake   [email protected]    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#7 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Eric Blake
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 5 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

hw/pci.c +3 -0
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow. If addr contains "100000000000000:0:0", there's no telling whether it will be diagnosed as a parse error, or silently accepted and truncated, in which case, there's no telling what dom will contain.

Quoted 8 lines
Quoted 6 lines

I cringe any time I see someone using scanf to parse numbers from arbitrary user input; I barely tolerate it for parsing things generated by the kernel, but even there, I won't ever use scanf myself. Same goes
for atoi. _Only_ strtol and friends can robustly parse arbitrary input into integers.

Seems easy to fix: I'll just set maximum field width of 8.

Nope. Functional change: "000000001.2" is no longer accepted.

Quoted 1 line

Any other issues?

Yes.

1. More functional changes, e.g.

* "1" is no longer rejected when funcp != NULL

Probably more. I'd be particularly wary of sscanf()'s appetite for space.

2. Diffstat: 1 files changed, 38 insertions(+), 43 deletions(-)

Why bother?

author
#8 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 26 lines · Markus Armbruster
Quoted 24 lines · Michael S. Tsirkin
Quoted 22 lines · Eric Blake
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 5 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

hw/pci.c +3 -0
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow. If addr contains "100000000000000:0:0", there's no telling whether it will be diagnosed as a parse error, or silently accepted and truncated, in which case, there's no telling what dom will contain.

I cringe any time I see someone using scanf to parse numbers from arbitrary user input; I barely tolerate it for parsing things generated by the kernel, but even there, I won't ever use scanf myself. Same goes
for atoi. _Only_ strtol and friends can robustly parse arbitrary input into integers.

Quoted 3 lines
Quoted 1 line

Seems easy to fix: I'll just set maximum field width of 8.

Nope. Functional change: "000000001.2" is no longer accepted.

Hmm. Why would anyone do this?
But I get the hint I think. So I think what I will do is, use sscanf to validate, detect the format and tokenize input, then strtoul to read in integers.

Quoted 7 lines
Quoted 1 line

Any other issues?

Yes.

1. More functional changes, e.g.

* "1" is no longer rejected when funcp != NULL

I think it's a bug. Why would we want to reject it?

Quoted 2 lines

Probably more. I'd be particularly wary of sscanf()'s appetite for space.

We pass in a pre-parsed string. How can it have spaces?

Quoted 3 lines

2. Diffstat: 1 files changed, 38 insertions(+), 43 deletions(-)

Why bother?

I need to extend the functionality and manual parsing makes it very fragile.

Signature · 1 line
MST
author
#9 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

In reply to Markus Armbruster
Quoted 26 lines · Markus Armbruster
Quoted 24 lines · Michael S. Tsirkin
Quoted 22 lines · Eric Blake
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 5 lines · Michael S. Tsirkin

Use scanf instead of manual string scanning.

hw/pci.c +3 -0
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow. If addr contains "100000000000000:0:0", there's no telling whether it will be diagnosed as a parse error, or silently accepted and truncated, in which case, there's no telling what dom will contain.

I cringe any time I see someone using scanf to parse numbers from arbitrary user input; I barely tolerate it for parsing things generated by the kernel, but even there, I won't ever use scanf myself. Same goes
for atoi. _Only_ strtol and friends can robustly parse arbitrary input into integers.

Quoted 3 lines
Quoted 1 line

Seems easy to fix: I'll just set maximum field width of 8.

Nope. Functional change: "000000001.2" is no longer accepted.

OK, there's an easy way to fix with
%*[0]%8x

Quoted 10 lines
Quoted 1 line

Any other issues?

Yes.

1. More functional changes, e.g.

* "1" is no longer rejected when funcp != NULL

Probably more. I'd be particularly wary of sscanf()'s appetite for space.

BTW strtoul that we currently use skips whitespace silently too. We probably should validate input with %*[0-9a-f.:] beforehand to address this.

Quoted 3 lines

2. Diffstat: 1 files changed, 38 insertions(+), 43 deletions(-)

Why bother?

author
#10 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

In reply to malc
Quoted 10 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 6 lines
hw/pci.c +6 -0
+{
+    unsigned dom, bus, slot, func;
+    int n = -1;
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Quoted 1 line

+ if (n == -1) {

If it does n will stay -1.

#11 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 6 lines
hw/pci.c +6 -0
+{
+    unsigned dom, bus, slot, func;
+    int n = -1;
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Quoted 5 lines
Quoted 1 line
Quoted 1 line

+ if (n == -1) {

If it does n will stay -1.

You are right. That does not, however, invalidate the points made by Eric Blake.

Signature · 1 line
mailto:[email protected]
author
#12 lore raw

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

Quoted 12 lines · malc
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · malc

hw/pci.c static void pci_set_default_subsystem_id(PCIDevice *pci_dev) · line 488

Quoted 6 lines
hw/pci.c +6 -0
+{
+    unsigned dom, bus, slot, func;
+    int n = -1;
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line

+ if (n == -1) {

If it does n will stay -1.

You are right. That does not, however, invalidate the points made by Eric Blake.

Yes. But I think
+ sscanf(addr, "%8x:%8x:%8x%n", &dom, &bus, &slot, &n);

addresses his points.

Quoted 2 lines
Signature · 1 line
mailto:[email protected]