[v3,34/52] xen/drivers/passthrough/pci.c: let custom parameter parsing routines return errno

Commit Message

Juergen Gross Aug. 16, 2017, 12:52 p.m. UTC

Modify the custom parameter parsing routines in:

xen/drivers/passthrough/pci.c

to indicate whether the parameter value was parsed successfully.

Cc: Jan Beulich <[e-mail]>
Signed-off-by: Juergen Gross <[e-mail]>
---
V3:
- cosmetic changes (Jan Beulich)
- dont modify option value in parsing funtion
---
 xen/drivers/passthrough/pci.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

Jan Beulich Aug. 22, 2017, 10:07 a.m. UTC | #1

>>> On 16.08.17 at 14:52, <[e-mail]> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -149,17 +149,18 @@ static struct phantom_dev {
>  } phantom_devs[8];
>  static unsigned int nr_phantom_devs;
>  
> -static void __init parse_phantom_dev(char *str) {
> +static int __init parse_phantom_dev(const char *str)
> +{
>      const char *s = str;
>      unsigned int seg, bus, slot;
>      struct phantom_dev phantom;
>  
>      if ( !s || !*s || nr_phantom_devs >= ARRAY_SIZE(phantom_devs) )
> -        return;
> +        return -EINVAL;

I think you want to split the conditional and return e.g. -E2BIG for
there being too many devices. You could then at once drop the
pointless !s.

Jan

Juergen Gross Aug. 23, 2017, 9:28 a.m. UTC | #2

On 22/08/17 12:07, Jan Beulich wrote:
>>>> On 16.08.17 at 14:52, <[e-mail]> wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -149,17 +149,18 @@ static struct phantom_dev {
>>  } phantom_devs[8];
>>  static unsigned int nr_phantom_devs;
>>  
>> -static void __init parse_phantom_dev(char *str) {
>> +static int __init parse_phantom_dev(const char *str)
>> +{
>>      const char *s = str;
>>      unsigned int seg, bus, slot;
>>      struct phantom_dev phantom;
>>  
>>      if ( !s || !*s || nr_phantom_devs >= ARRAY_SIZE(phantom_devs) )
>> -        return;
>> +        return -EINVAL;
> 
> I think you want to split the conditional and return e.g. -E2BIG for
> there being too many devices. You could then at once drop the
> pointless !s.

Yes.


Juergen

Patch

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27bdb7163c..fdc53aef12 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -149,17 +149,18 @@  static struct phantom_dev {
 } phantom_devs[8];
 static unsigned int nr_phantom_devs;
 
-static void __init parse_phantom_dev(char *str) {
+static int __init parse_phantom_dev(const char *str)
+{
     const char *s = str;
     unsigned int seg, bus, slot;
     struct phantom_dev phantom;
 
     if ( !s || !*s || nr_phantom_devs >= ARRAY_SIZE(phantom_devs) )
-        return;
+        return -EINVAL;
 
     s = parse_pci(s, &seg, &bus, &slot, NULL);
     if ( !s || *s != ',' )
-        return;
+        return -EINVAL;
 
     phantom.seg = seg;
     phantom.bus = bus;
@@ -170,10 +171,12 @@  static void __init parse_phantom_dev(char *str) {
     case 1: case 2: case 4:
         if ( *s )
     default:
-            return;
+            return -EINVAL;
     }
 
     phantom_devs[nr_phantom_devs++] = phantom;
+
+    return 0;
 }
 custom_param("pci-phantom", parse_phantom_dev);
 
@@ -189,9 +192,10 @@  static u16 __read_mostly bridge_ctl_mask;
  *   perr                       don't suppress parity errors (default)
  *   no-perr                    suppress parity errors
  */
-static void __init parse_pci_param(char *s)
+static int __init parse_pci_param(const char *s)
 {
-    char *ss;
+    const char *ss;
+    int rc = 0;
 
     do {
         bool_t on = !!strncmp(s, "no-", 3);
@@ -201,19 +205,21 @@  static void __init parse_pci_param(char *s)
             s += 3;
 
         ss = strchr(s, ',');
-        if ( ss )
-            *ss = '\0';
+        if ( !ss )
+            ss = strchr(s, '\0');
 
-        if ( !strcmp(s, "serr") )
+        if ( !strncmp(s, "serr", ss - s) )
         {
             cmd_mask = PCI_COMMAND_SERR;
             brctl_mask = PCI_BRIDGE_CTL_SERR | PCI_BRIDGE_CTL_DTMR_SERR;
         }
-        else if ( !strcmp(s, "perr") )
+        else if ( !strncmp(s, "perr", ss - s) )
         {
             cmd_mask = PCI_COMMAND_PARITY;
             brctl_mask = PCI_BRIDGE_CTL_PARITY;
         }
+        else
+            rc = -EINVAL;
 
         if ( on )
         {
@@ -227,7 +233,9 @@  static void __init parse_pci_param(char *s)
         }
 
         s = ss + 1;
-    } while ( ss );
+    } while ( *ss );
+
+    return rc;
 }
 custom_param("pci", parse_pci_param);