[Qemu-devel] [PATCH v2] error: passing a negative value to an os_errno is wrong

[Qemu-devel] [PATCH v2] error: passing a negative value to an os_errno is wrong

[SeokYeon Hwang]

Added 'assert(os_errno > 0)' in 'error_set_errno()'.
Fixed errno since it passes wrong value to 'error_set_errno()'.

Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
---
 hw/pci/pcie.c | 2 +-
 util/error.c  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 58455bd..2902f7d 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -229,7 +229,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
         /* the slot is electromechanically locked.
          * This error is propagated up to qdev and then to HMP/QMP.
          */
-        error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
+        error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
     }
 }
 
diff --git a/util/error.c b/util/error.c
index 2ace0d8..4ce22cc 100644
--- a/util/error.c
+++ b/util/error.c
@@ -62,6 +62,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
         return;
     }
     assert(*errp == NULL);
+    assert(os_errno > 0);
 
     err = g_malloc0(sizeof(*err));
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2] error: passing a negative value to an os_errno is wrong

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Fri, Nov 07, 2014 at 11:24:55AM +0900, SeokYeon Hwang wrote:
> Added 'assert(os_errno > 0)' in 'error_set_errno()'.
> Fixed errno since it passes wrong value to 'error_set_errno()'.
> 
> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> ---
>  hw/pci/pcie.c | 2 +-
>  util/error.c  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 58455bd..2902f7d 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -229,7 +229,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
>          /* the slot is electromechanically locked.
>           * This error is propagated up to qdev and then to HMP/QMP.
>           */
> -        error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
> +        error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
>      }
>  }
>  
> diff --git a/util/error.c b/util/error.c
> index 2ace0d8..4ce22cc 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -62,6 +62,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>          return;
>      }
>      assert(*errp == NULL);
> +    assert(os_errno > 0);

strerror(0) will return string 'Success', do we need to reserve zero here?

      assert(os_errno >= 0);

>  
>      err = g_malloc0(sizeof(*err));
>  
> -- 
> 2.1.0
> 

-- 
			Amos.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] error: passing a negative value to an os_errno is wrong

[Markus Armbruster]

Amos Kong <kongjianjun@gmail.com> writes:

> On Fri, Nov 07, 2014 at 11:24:55AM +0900, SeokYeon Hwang wrote:
>> Added 'assert(os_errno > 0)' in 'error_set_errno()'.
>> Fixed errno since it passes wrong value to 'error_set_errno()'.
>> 
>> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
>> ---
>>  hw/pci/pcie.c | 2 +-
>>  util/error.c  | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 58455bd..2902f7d 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -229,7 +229,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
>>          /* the slot is electromechanically locked.
>>           * This error is propagated up to qdev and then to HMP/QMP.
>>           */
>> -        error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
>> +        error_setg_errno(errp, EBUSY, "slot is electromechanically locked");
>>      }
>>  }
>>  
>> diff --git a/util/error.c b/util/error.c
>> index 2ace0d8..4ce22cc 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -62,6 +62,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>>          return;
>>      }
>>      assert(*errp == NULL);
>> +    assert(os_errno > 0);
>
> strerror(0) will return string 'Success', do we need to reserve zero here?
>
>       assert(os_errno >= 0);

Yes, because...

>>      err = g_malloc0(sizeof(*err));
>>  
        va_start(ap, fmt);
        msg1 = g_strdup_vprintf(fmt, ap);
--->    if (os_errno != 0) {
            err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
            g_free(msg1);
        } else {
            err->msg = msg1;
        }

Please fix.

Re: [Qemu-devel] [PATCH v2] error: passing a negative value to an os_errno is wrong

[SeokYeon Hwang]

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Friday, November 07, 2014 4:41 PM
> To: Amos Kong
> Cc: SeokYeon Hwang; mreitz@redhat.com; paolo.bonzini@gmail.com; qemu-
> devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v2] error: passing a negative value to an
> os_errno is wrong
> 
> Amos Kong <kongjianjun@gmail.com> writes:
> 
> > On Fri, Nov 07, 2014 at 11:24:55AM +0900, SeokYeon Hwang wrote:
> >> Added 'assert(os_errno > 0)' in 'error_set_errno()'.
> >> Fixed errno since it passes wrong value to 'error_set_errno()'.
> >>
> >> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> >> ---
> >>  hw/pci/pcie.c | 2 +-
> >>  util/error.c  | 1 +
> >>  2 files changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 58455bd..2902f7d
> >> 100644
> >> --- a/hw/pci/pcie.c
> >> +++ b/hw/pci/pcie.c
> >> @@ -229,7 +229,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice
> *hotplug_dev,
> >>          /* the slot is electromechanically locked.
> >>           * This error is propagated up to qdev and then to HMP/QMP.
> >>           */
> >> -        error_setg_errno(errp, -EBUSY, "slot is electromechanically
> locked");
> >> +        error_setg_errno(errp, EBUSY, "slot is electromechanically
> >> + locked");
> >>      }
> >>  }
> >>
> >> diff --git a/util/error.c b/util/error.c index 2ace0d8..4ce22cc
> >> 100644
> >> --- a/util/error.c
> >> +++ b/util/error.c
> >> @@ -62,6 +62,7 @@ void error_set_errno(Error **errp, int os_errno,
> ErrorClass err_class,
> >>          return;
> >>      }
> >>      assert(*errp == NULL);
> >> +    assert(os_errno > 0);
> >
> > strerror(0) will return string 'Success', do we need to reserve zero
> here?
> >
> >       assert(os_errno >= 0);
> 
> Yes, because...
> 
> >>      err = g_malloc0(sizeof(*err));
> >>
>         va_start(ap, fmt);
>         msg1 = g_strdup_vprintf(fmt, ap);
> --->    if (os_errno != 0) {
>             err->msg = g_strdup_printf("%s: %s", msg1,
strerror(os_errno));
>             g_free(msg1);
>         } else {
>             err->msg = msg1;
>         }
> 
> Please fix.

You're right. I posted patch v3.
Thank you for your advice.