[Qemu-devel] [PATCH 0/3] unchecked uses of strdup

[Qemu-devel] [PATCH 0/3] unchecked uses of strdup

[jim]

From: Jim Meyering <meyering@redhat.com>

I've been auditing for unchecked uses of strdup.
Here are fixes for a few:

Jim Meyering (3):
  envlist.c: handle strdup failure
  scsi,pci,qdev,isa-bus,sysbus: don't let *_get_fw_dev_path return NULL
  sparc: use g_strdup in place of unchecked strdup

 envlist.c          | 9 ++++++++-
 hw/ide/qdev.c      | 2 +-
 hw/isa-bus.c       | 2 +-
 hw/pci.c           | 2 +-
 hw/qdev.c          | 2 +-
 hw/scsi-bus.c      | 8 ++------
 hw/sysbus.c        | 2 +-
 target-sparc/cpu.c | 4 ++--
 8 files changed, 17 insertions(+), 14 deletions(-)

--
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[jim]

From: Jim Meyering <meyering@redhat.com>

Without this, envlist_to_environ may silently fail to copy all
strings into the destination buffer, and both callers would leak
any env strings allocated after a failing strdup, because the
freeing code stops at the first NULL pointer.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 envlist.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/envlist.c b/envlist.c
index f2303cd..2bbd99c 100644
--- a/envlist.c
+++ b/envlist.c
@@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)

 	for (entry = envlist->el_entries.lh_first; entry != NULL;
 	    entry = entry->ev_link.le_next) {
-		*(penv++) = strdup(entry->ev_var);
+		if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
+			char **e = env;
+			while (e != penv) {
+				free(*e++);
+			}
+			free(env);
+			return NULL;
+		}
 	}
 	*penv = NULL; /* NULL terminate the list */

-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH 2/3] scsi, pci, qdev, isa-bus, sysbus: don't let *_get_fw_dev_path return NULL

[jim]

From: Jim Meyering <meyering@redhat.com>

Use g_strdup rather than strdup, because the sole caller
(qdev_get_fw_dev_path_helper) assumes it gets non-NULL, and dereferences
it.  Besides, in that caller, the allocated buffer is already freed with
g_free, so it's better to allocate with a matching g_strdup.

In one case, (scsi-bus.c) it was trivial, so I replaced an snprintf+
g_strdup combination with an equivalent g_strdup_printf use.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/ide/qdev.c | 2 +-
 hw/isa-bus.c  | 2 +-
 hw/pci.c      | 2 +-
 hw/qdev.c     | 2 +-
 hw/scsi-bus.c | 8 ++------
 hw/sysbus.c   | 2 +-
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index a46578d..9edfd8b 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -50,7 +50,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
     snprintf(path, sizeof(path), "%s@%d", qdev_fw_name(dev),
              ((IDEBus*)dev->parent_bus)->bus_id);

-    return strdup(path);
+    return g_strdup(path);
 }

 static int ide_qdev_init(DeviceState *qdev)
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 5a43f03..822b040 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -227,7 +227,7 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
         snprintf(path + off, sizeof(path) - off, "@%04x", d->ioport_id);
     }

-    return strdup(path);
+    return g_strdup(path);
 }

 MemoryRegion *isa_address_space(ISADevice *dev)
diff --git a/hw/pci.c b/hw/pci.c
index b706e69..dc21631 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1898,7 +1898,7 @@ static char *pcibus_get_fw_dev_path(DeviceState *dev)
                    PCI_SLOT(d->devfn));
     if (PCI_FUNC(d->devfn))
         snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
-    return strdup(path);
+    return g_strdup(path);
 }

 static char *pcibus_get_dev_path(DeviceState *dev)
diff --git a/hw/qdev.c b/hw/qdev.c
index 6a8f6bd..ab299cf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -512,7 +512,7 @@ char* qdev_get_fw_dev_path(DeviceState *dev)

     path[l-1] = '\0';

-    return strdup(path);
+    return g_strdup(path);
 }

 static char *qdev_get_type(Object *obj, Error **errp)
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 8ab9bcd..a4e93d1 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1470,12 +1470,8 @@ static char *scsibus_get_dev_path(DeviceState *dev)
 static char *scsibus_get_fw_dev_path(DeviceState *dev)
 {
     SCSIDevice *d = SCSI_DEVICE(dev);
-    char path[100];
-
-    snprintf(path, sizeof(path), "channel@%x/%s@%x,%x", d->channel,
-             qdev_fw_name(dev), d->id, d->lun);
-
-    return strdup(path);
+    return g_strdup_printf("channel@%x/%s@%x,%x", d->channel,
+                           qdev_fw_name(dev), d->id, d->lun);
 }

 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index db4efcc..8a9b85d 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -203,7 +203,7 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
         snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
     }

-    return strdup(path);
+    return g_strdup(path);
 }

 void sysbus_add_memory(SysBusDevice *dev, target_phys_addr_t addr,
-- 
1.7.10.2.484.gcd07cc5

[Qemu-devel] [PATCH 3/3] sparc: use g_strdup in place of unchecked strdup

[jim]

From: Jim Meyering <meyering@redhat.com>

This avoids a NULL-deref upon strdup failure.
Also update matching free to g_free.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 target-sparc/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 7ac6bdb..1e31318 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -648,7 +648,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 {
     unsigned int i;
     const sparc_def_t *def = NULL;
-    char *s = strdup(cpu_model);
+    char *s = g_strdup(cpu_model);
     char *featurestr, *name = strtok(s, ",");
     uint32_t plus_features = 0;
     uint32_t minus_features = 0;
@@ -740,7 +740,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 #ifdef DEBUG_FEATURES
     print_features(stderr, fprintf, cpu_def->features, NULL);
 #endif
-    free(s);
+    g_free(s);
     return 0;

  error:
-- 
1.7.10.2.484.gcd07cc5

Re: [Qemu-devel] [PATCH 2/3] scsi, pci, qdev, isa-bus, sysbus: don't let *_get_fw_dev_path return NULL

[Paolo Bonzini]

Il 15/05/2012 15:04, jim@meyering.net ha scritto:
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 8ab9bcd..a4e93d1 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -1470,12 +1470,8 @@ static char *scsibus_get_dev_path(DeviceState *dev)
>  static char *scsibus_get_fw_dev_path(DeviceState *dev)
>  {
>      SCSIDevice *d = SCSI_DEVICE(dev);
> -    char path[100];
> -
> -    snprintf(path, sizeof(path), "channel@%x/%s@%x,%x", d->channel,
> -             qdev_fw_name(dev), d->id, d->lun);
> -
> -    return strdup(path);
> +    return g_strdup_printf("channel@%x/%s@%x,%x", d->channel,
> +                           qdev_fw_name(dev), d->id, d->lun);
>  }
> 
>  SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I'm assuming that these will be handled by a global committer.

Paolo

Re: [Qemu-devel] [PATCH 2/3] scsi, pci, qdev, isa-bus, sysbus: don't let *_get_fw_dev_path return NULL

[Kevin Wolf]

Am 15.05.2012 15:04, schrieb jim@meyering.net:
> From: Jim Meyering <meyering@redhat.com>
> 
> Use g_strdup rather than strdup, because the sole caller
> (qdev_get_fw_dev_path_helper) assumes it gets non-NULL, and dereferences
> it.  Besides, in that caller, the allocated buffer is already freed with
> g_free, so it's better to allocate with a matching g_strdup.
> 
> In one case, (scsi-bus.c) it was trivial, so I replaced an snprintf+
> g_strdup combination with an equivalent g_strdup_printf use.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Blue Swirl]

On Tue, May 15, 2012 at 1:04 PM,  <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Without this, envlist_to_environ may silently fail to copy all
> strings into the destination buffer, and both callers would leak
> any env strings allocated after a failing strdup, because the
> freeing code stops at the first NULL pointer.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  envlist.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/envlist.c b/envlist.c
> index f2303cd..2bbd99c 100644
> --- a/envlist.c
> +++ b/envlist.c
> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
>
>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>            entry = entry->ev_link.le_next) {
> -               *(penv++) = strdup(entry->ev_var);
> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
> +                       char **e = env;
> +                       while (e != penv) {
> +                               free(*e++);
> +                       }
> +                       free(env);
> +                       return NULL;
> +               }

ERROR: code indent should never use tabs
#82: FILE: envlist.c:238:
+^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$

ERROR: do not use assignment in if condition
#82: FILE: envlist.c:238:
+               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {

ERROR: code indent should never use tabs
#83: FILE: envlist.c:239:
+^I^I^Ichar **e = env;$

ERROR: code indent should never use tabs
#84: FILE: envlist.c:240:
+^I^I^Iwhile (e != penv) {$

ERROR: code indent should never use tabs
#85: FILE: envlist.c:241:
+^I^I^I^Ifree(*e++);$

ERROR: code indent should never use tabs
#86: FILE: envlist.c:242:
+^I^I^I}$

ERROR: code indent should never use tabs
#87: FILE: envlist.c:243:
+^I^I^Ifree(env);$

ERROR: code indent should never use tabs
#88: FILE: envlist.c:244:
+^I^I^Ireturn NULL;$

ERROR: code indent should never use tabs
#89: FILE: envlist.c:245:
+^I^I}$

total: 9 errors, 0 warnings, 15 lines checked

Please fix.

>        }
>        *penv = NULL; /* NULL terminate the list */
>
> --
> 1.7.10.2.484.gcd07cc5
>
>

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Jim Meyering]

Blue Swirl wrote:
> On Tue, May 15, 2012 at 1:04 PM,  <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Without this, envlist_to_environ may silently fail to copy all
>> strings into the destination buffer, and both callers would leak
>> any env strings allocated after a failing strdup, because the
>> freeing code stops at the first NULL pointer.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  envlist.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/envlist.c b/envlist.c
>> index f2303cd..2bbd99c 100644
>> --- a/envlist.c
>> +++ b/envlist.c
>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
>>
>>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>>            entry = entry->ev_link.le_next) {
>> -               *(penv++) = strdup(entry->ev_var);
>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>> +                       char **e = env;
>> +                       while (e != penv) {
>> +                               free(*e++);
>> +                       }
>> +                       free(env);
>> +                       return NULL;
>> +               }
>
> ERROR: code indent should never use tabs
> #82: FILE: envlist.c:238:
> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$

That entire file is indented solely with TABs, so adding these new
lines using spaces for indentation seems unjustified: the mix tends
to make the code unreadable in some contexts (email quoting, for one).
How about two patches: one to convert all leading TABs in envlist.c to
spaces, and the next to make the above change, but indenting with spaces?

> ERROR: do not use assignment in if condition
> #82: FILE: envlist.c:238:
> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {

I agree with the sentiment, but found that the alternative was less
readable and less maintainable, since I'd have to increment "penv" in
two places (both in the if-block and after it) rather than in just one.
However, I've just realized I can hoist the "penv++" increment into the
"for-statement", in which case it's ok:

        for (entry = envlist->el_entries.lh_first; entry != NULL;
             entry = entry->ev_link.le_next, penv++) {
                *penv = strdup(entry->ev_var);
                if (*penv == NULL) {
                        char **e = env;
                        while (e <= penv) {
                                free(*e++);
                        }
                        free(env);
                        return NULL;
                }
        }

Your move.  Which would you prefer?
  1) two patches: one replacing all leading TABs with equivalent spaces,
      then the above patch
  2) one patch, indented using TABs, in spite of the checkpatch failure
  3) one patch, indented using spaces, in spite of the consistency issue

...
> total: 9 errors, 0 warnings, 15 lines checked
>
> Please fix.

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Blue Swirl]

On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <jim@meyering.net> wrote:
> Blue Swirl wrote:
>> On Tue, May 15, 2012 at 1:04 PM,  <jim@meyering.net> wrote:
>>> From: Jim Meyering <meyering@redhat.com>
>>>
>>> Without this, envlist_to_environ may silently fail to copy all
>>> strings into the destination buffer, and both callers would leak
>>> any env strings allocated after a failing strdup, because the
>>> freeing code stops at the first NULL pointer.
>>>
>>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>>> ---
>>>  envlist.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/envlist.c b/envlist.c
>>> index f2303cd..2bbd99c 100644
>>> --- a/envlist.c
>>> +++ b/envlist.c
>>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
>>>
>>>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>>>            entry = entry->ev_link.le_next) {
>>> -               *(penv++) = strdup(entry->ev_var);
>>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>>> +                       char **e = env;
>>> +                       while (e != penv) {
>>> +                               free(*e++);
>>> +                       }
>>> +                       free(env);
>>> +                       return NULL;
>>> +               }
>>
>> ERROR: code indent should never use tabs
>> #82: FILE: envlist.c:238:
>> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$
>
> That entire file is indented solely with TABs, so adding these new
> lines using spaces for indentation seems unjustified: the mix tends
> to make the code unreadable in some contexts (email quoting, for one).
> How about two patches: one to convert all leading TABs in envlist.c to
> spaces, and the next to make the above change, but indenting with spaces?
>
>> ERROR: do not use assignment in if condition
>> #82: FILE: envlist.c:238:
>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>
> I agree with the sentiment, but found that the alternative was less
> readable and less maintainable, since I'd have to increment "penv" in
> two places (both in the if-block and after it) rather than in just one.
> However, I've just realized I can hoist the "penv++" increment into the
> "for-statement", in which case it's ok:
>
>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>             entry = entry->ev_link.le_next, penv++) {
>                *penv = strdup(entry->ev_var);
>                if (*penv == NULL) {
>                        char **e = env;
>                        while (e <= penv) {
>                                free(*e++);
>                        }
>                        free(env);
>                        return NULL;
>                }
>        }
>
> Your move.  Which would you prefer?
>  1) two patches: one replacing all leading TABs with equivalent spaces,
>      then the above patch
>  2) one patch, indented using TABs, in spite of the checkpatch failure
>  3) one patch, indented using spaces, in spite of the consistency issue

1) (or 3). Though for v1.1, maybe 3) is the smaller fix and later do 1) for 1.2.

>
> ...
>> total: 9 errors, 0 warnings, 15 lines checked
>>
>> Please fix.

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Kevin Wolf]

Am 21.05.2012 19:56, schrieb Blue Swirl:
> On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <jim@meyering.net> wrote:
>> Blue Swirl wrote:
>>> On Tue, May 15, 2012 at 1:04 PM,  <jim@meyering.net> wrote:
>>>> From: Jim Meyering <meyering@redhat.com>
>>>>
>>>> Without this, envlist_to_environ may silently fail to copy all
>>>> strings into the destination buffer, and both callers would leak
>>>> any env strings allocated after a failing strdup, because the
>>>> freeing code stops at the first NULL pointer.
>>>>
>>>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>>>> ---
>>>>  envlist.c | 9 ++++++++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/envlist.c b/envlist.c
>>>> index f2303cd..2bbd99c 100644
>>>> --- a/envlist.c
>>>> +++ b/envlist.c
>>>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count)
>>>>
>>>>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>>>>            entry = entry->ev_link.le_next) {
>>>> -               *(penv++) = strdup(entry->ev_var);
>>>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>>>> +                       char **e = env;
>>>> +                       while (e != penv) {
>>>> +                               free(*e++);
>>>> +                       }
>>>> +                       free(env);
>>>> +                       return NULL;
>>>> +               }
>>>
>>> ERROR: code indent should never use tabs
>>> #82: FILE: envlist.c:238:
>>> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$
>>
>> That entire file is indented solely with TABs, so adding these new
>> lines using spaces for indentation seems unjustified: the mix tends
>> to make the code unreadable in some contexts (email quoting, for one).
>> How about two patches: one to convert all leading TABs in envlist.c to
>> spaces, and the next to make the above change, but indenting with spaces?
>>
>>> ERROR: do not use assignment in if condition
>>> #82: FILE: envlist.c:238:
>>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>>
>> I agree with the sentiment, but found that the alternative was less
>> readable and less maintainable, since I'd have to increment "penv" in
>> two places (both in the if-block and after it) rather than in just one.
>> However, I've just realized I can hoist the "penv++" increment into the
>> "for-statement", in which case it's ok:
>>
>>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>>             entry = entry->ev_link.le_next, penv++) {
>>                *penv = strdup(entry->ev_var);
>>                if (*penv == NULL) {
>>                        char **e = env;
>>                        while (e <= penv) {
>>                                free(*e++);
>>                        }
>>                        free(env);
>>                        return NULL;
>>                }
>>        }
>>
>> Your move.  Which would you prefer?
>>  1) two patches: one replacing all leading TABs with equivalent spaces,
>>      then the above patch
>>  2) one patch, indented using TABs, in spite of the checkpatch failure
>>  3) one patch, indented using spaces, in spite of the consistency issue
> 
> 1) (or 3). Though for v1.1, maybe 3) is the smaller fix and later do 1) for 1.2.

A patch replacing tabs by spaces isn't really the kind of patches that
we would want to avoid during freeze. It's easy enough to check with git
diff -w that it doesn't change anything semantically.

Kevin

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Jim Meyering]

Kevin Wolf wrote:
> A patch replacing tabs by spaces isn't really the kind of patches that
> we would want to avoid during freeze. It's easy enough to check with git
> diff -w that it doesn't change anything semantically.

That makes sense, so I've posted two patches:

    1) two patches: one replacing all leading TABs with equivalent spaces,
          then the above patch

Note however, that envlist.c must predate checkpatch.pl, since
that patch provokes numerous warnings about style issues:
(hmm... I've just noticed these first two, which seem to suggest
that even for comments I should remove the TABs.  Is it worth
a V2 to fix those two lines?  Or are they false positives, since
they're not really "code" indentation?  )

ERROR: code indent should never use tabs
#23: FILE: envlist.c:11:
+        const char *ev_var;^I^I^I/* actual env value */$

ERROR: code indent should never use tabs
#30: FILE: envlist.c:16:
+        QLIST_HEAD(, envlist_entry) el_entries;^I/* actual entries */$

ERROR: code indent should never use tabs
#31: FILE: envlist.c:17:
+        size_t el_count;^I^I^I/* number of entries */$

WARNING: space prohibited between function name and open parenthesis '('
#44: FILE: envlist.c:32:
+        if ((envlist = malloc(sizeof (*envlist))) == NULL)

ERROR: do not use assignment in if condition
#44: FILE: envlist.c:32:
+        if ((envlist = malloc(sizeof (*envlist))) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#44: FILE: envlist.c:32:
+        if ((envlist = malloc(sizeof (*envlist))) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#45: FILE: envlist.c:33:
+                return (NULL);

ERROR: return is not a function, parentheses are not required
#53: FILE: envlist.c:38:
+        return (envlist);

ERROR: return is not a function, parentheses are not required
#90: FILE: envlist.c:75:
+        return (envlist_parse(envlist, env, &envlist_setenv));

ERROR: return is not a function, parentheses are not required
#99: FILE: envlist.c:87:
+        return (envlist_parse(envlist, env, &envlist_unsetenv));

WARNING: braces {} are necessary for all arms of this statement
#138: FILE: envlist.c:105:
+        if ((envlist == NULL) || (env == NULL))
[...]

ERROR: return is not a function, parentheses are not required
#139: FILE: envlist.c:106:
+                return (EINVAL);

ERROR: do not use assignment in if condition
#145: FILE: envlist.c:112:
+        if ((tmpenv = strdup(env)) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#145: FILE: envlist.c:112:
+        if ((tmpenv = strdup(env)) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#146: FILE: envlist.c:113:
+                return (errno);

ERROR: return is not a function, parentheses are not required
#152: FILE: envlist.c:119:
+                        return (errno);

ERROR: return is not a function, parentheses are not required
#158: FILE: envlist.c:125:
+        return (0);

WARNING: braces {} are necessary for all arms of this statement
#210: FILE: envlist.c:141:
+        if ((envlist == NULL) || (env == NULL))
[...]

ERROR: return is not a function, parentheses are not required
#211: FILE: envlist.c:142:
+                return (EINVAL);

ERROR: do not use assignment in if condition
#214: FILE: envlist.c:145:
+        if ((eq_sign = strchr(env, '=')) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#214: FILE: envlist.c:145:
+        if ((eq_sign = strchr(env, '=')) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#215: FILE: envlist.c:146:
+                return (EINVAL);

WARNING: braces {} are necessary for all arms of this statement
#225: FILE: envlist.c:156:
+                if (strncmp(entry->ev_var, env, envname_len) == 0)
[...]

WARNING: space prohibited between function name and open parenthesis '('
#237: FILE: envlist.c:168:
+        if ((entry = malloc(sizeof (*entry))) == NULL)

ERROR: do not use assignment in if condition
#237: FILE: envlist.c:168:
+        if ((entry = malloc(sizeof (*entry))) == NULL)

WARNING: braces {} are necessary for all arms of this statement
#237: FILE: envlist.c:168:
+        if ((entry = malloc(sizeof (*entry))) == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#238: FILE: envlist.c:169:
+                return (errno);

ERROR: do not use assignment in if condition
#239: FILE: envlist.c:170:
+        if ((entry->ev_var = strdup(env)) == NULL) {

ERROR: return is not a function, parentheses are not required
#241: FILE: envlist.c:172:
+                return (errno);

ERROR: return is not a function, parentheses are not required
#245: FILE: envlist.c:176:
+        return (0);

WARNING: braces {} are necessary for all arms of this statement
#284: FILE: envlist.c:189:
+        if ((envlist == NULL) || (env == NULL))
[...]

ERROR: return is not a function, parentheses are not required
#285: FILE: envlist.c:190:
+                return (EINVAL);

WARNING: braces {} are necessary for all arms of this statement
#288: FILE: envlist.c:193:
+        if (strchr(env, '=') != NULL)
[...]

ERROR: return is not a function, parentheses are not required
#289: FILE: envlist.c:194:
+                return (EINVAL);

WARNING: braces {} are necessary for all arms of this statement
#298: FILE: envlist.c:203:
+                if (strncmp(entry->ev_var, env, envname_len) == 0)
[...]

ERROR: return is not a function, parentheses are not required
#308: FILE: envlist.c:213:
+        return (0);

WARNING: space prohibited between function name and open parenthesis '('
#324: FILE: envlist.c:232:
+        penv = env = malloc((envlist->el_count + 1) * sizeof (char *));

WARNING: braces {} are necessary for all arms of this statement
#325: FILE: envlist.c:233:
+        if (env == NULL)
[...]

ERROR: return is not a function, parentheses are not required
#326: FILE: envlist.c:234:
+                return (NULL);

WARNING: braces {} are necessary for all arms of this statement
#341: FILE: envlist.c:242:
+        if (count != NULL)
[...]

ERROR: return is not a function, parentheses are not required
#345: FILE: envlist.c:245:
+        return (env);

total: 26 errors, 15 warnings, 321 lines checked

.qe-strdup/0001-envlist.c-convert-many-leading-TABs-to-spaces-via-ex.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
total: 0 errors, 0 warnings, 18 lines checked

.qe-strdup/0002-envlist.c-handle-strdup-failure.patch has no obvious style problems and is ready for submission.

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Kevin Wolf]

Am 22.05.2012 11:05, schrieb Jim Meyering:
> Kevin Wolf wrote:
>> A patch replacing tabs by spaces isn't really the kind of patches that
>> we would want to avoid during freeze. It's easy enough to check with git
>> diff -w that it doesn't change anything semantically.
> 
> That makes sense, so I've posted two patches:
> 
>     1) two patches: one replacing all leading TABs with equivalent spaces,
>           then the above patch
> 
> Note however, that envlist.c must predate checkpatch.pl, since
> that patch provokes numerous warnings about style issues:

Quite possible. Most files in qemu will still contain some violations.

> (hmm... I've just noticed these first two, which seem to suggest
> that even for comments I should remove the TABs.  Is it worth
> a V2 to fix those two lines?  Or are they false positives, since
> they're not really "code" indentation?  )

We don't use tabs at all where possible. There shouldn't be much more
exceptions than the Makefiles.

Kevin

Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Jim Meyering]

Kevin Wolf wrote:

> Am 22.05.2012 11:05, schrieb Jim Meyering:
>> Kevin Wolf wrote:
>>> A patch replacing tabs by spaces isn't really the kind of patches that
>>> we would want to avoid during freeze. It's easy enough to check with git
>>> diff -w that it doesn't change anything semantically.
>>
>> That makes sense, so I've posted two patches:
>>
>>     1) two patches: one replacing all leading TABs with equivalent spaces,
>>           then the above patch
>>
>> Note however, that envlist.c must predate checkpatch.pl, since
>> that patch provokes numerous warnings about style issues:
>
> Quite possible. Most files in qemu will still contain some violations.
>
>> (hmm... I've just noticed these first two, which seem to suggest
>> that even for comments I should remove the TABs.  Is it worth
>> a V2 to fix those two lines?  Or are they false positives, since
>> they're not really "code" indentation?  )
>
> We don't use tabs at all where possible. There shouldn't be much more
> exceptions than the Makefiles.

Sounds like a v2 would be welcome.
Here you go...