diff mbox

[v3] error: passing a negative value to an os_errno is wrong

Message ID 1415598889-11244-1-git-send-email-syeon.hwang@samsung.com
State New
Headers show

Commit Message

SeokYeon Hwang Nov. 10, 2014, 5:54 a.m. UTC 
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(-)

Comments

Jianjun Kong Nov. 10, 2014, 6:32 a.m. UTC | #1 
On Mon, Nov 10, 2014 at 02:54:49PM +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>

Reviewed-by: Amos Kong <kongjianjun@gmail.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..6c9d995 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
>
Max Reitz Nov. 10, 2014, 8:36 a.m. UTC | #2 
On 2014-11-10 at 06:54, 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..6c9d995 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));
>   

Reviewed-by: Max Reitz <mreitz@redhat.com>
Markus Armbruster Nov. 10, 2014, 9:32 a.m. UTC | #3 
SeokYeon Hwang <syeon.hwang@samsung.com> writes:

> 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..6c9d995 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));

The first hunk could still go into 2.2 as a bug fix.  The rest can't.
You could post just the first hunk as "[PATCH for-2.2] pci: Don't pass
negative errno to error_set_errno()", with my R-by.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
SeokYeon Hwang Nov. 10, 2014, 12:57 p.m. UTC | #4 
> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Monday, November 10, 2014 6:33 PM
> To: SeokYeon Hwang
> Cc: qemu-devel@nongnu.org; kongjianjun@gmail.com; paolo.bonzini@gmail.com;
> mreitz@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v3] error: passing a negative value to an
> os_errno is wrong
> 
> SeokYeon Hwang <syeon.hwang@samsung.com> writes:
> 
> > 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..6c9d995 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));
> 
> The first hunk could still go into 2.2 as a bug fix.  The rest can't.
> You could post just the first hunk as "[PATCH for-2.2] pci: Don't pass
> negative errno to error_set_errno()", with my R-by.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Did you mean separate this patch into two patches ??
One is "PATCH v4" and the other is "PATCH for-2.2". Am I right??

And I have a question. (I don't know review / merge process well.)
What happens to the "reviewed" but "not bug-fix" patch during "feature
freeze" time ??

Thanks.
Markus Armbruster Nov. 10, 2014, 1:20 p.m. UTC | #5 
SeokYeon Hwang <syeon.hwang@samsung.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster [mailto:armbru@redhat.com]
>> Sent: Monday, November 10, 2014 6:33 PM
>> To: SeokYeon Hwang
>> Cc: qemu-devel@nongnu.org; kongjianjun@gmail.com; paolo.bonzini@gmail.com;
>> mreitz@redhat.com
>> Subject: Re: [Qemu-devel] [PATCH v3] error: passing a negative value to an
>> os_errno is wrong
>> 
>> SeokYeon Hwang <syeon.hwang@samsung.com> writes:
>> 
>> > 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..6c9d995 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));
>> 
>> The first hunk could still go into 2.2 as a bug fix.  The rest can't.
>> You could post just the first hunk as "[PATCH for-2.2] pci: Don't pass
>> negative errno to error_set_errno()", with my R-by.
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Did you mean separate this patch into two patches ??
> One is "PATCH v4" and the other is "PATCH for-2.2". Am I right??

Yes.

> And I have a question. (I don't know review / merge process well.)
> What happens to the "reviewed" but "not bug-fix" patch during "feature
> freeze" time ??

They get applied when the next development cycle opens.  If they don't,
you have to remind the maintainer(s).
Eric Blake Nov. 10, 2014, 4:14 p.m. UTC | #6 
On 11/09/2014 10:54 PM, SeokYeon Hwang wrote:
> Added 'assert(os_errno > 0)' in 'error_set_errno()'.

Commit message...

> 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 +-

> +++ 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);

...does not match code. (Code is correct).
SeokYeon Hwang Nov. 12, 2014, 7 a.m. UTC | #7 
> -----Original Message-----
> From: Eric Blake [mailto:eblake@redhat.com]
> Sent: Tuesday, November 11, 2014 1:14 AM
> To: SeokYeon Hwang; qemu-devel@nongnu.org
> Cc: paolo.bonzini@gmail.com; armbru@redhat.com; mreitz@redhat.com;
> kongjianjun@gmail.com
> Subject: Re: [PATCH v3] error: passing a negative value to an os_errno is
> wrong
> 
> On 11/09/2014 10:54 PM, SeokYeon Hwang wrote:
> > Added 'assert(os_errno > 0)' in 'error_set_errno()'.
> 
> Commit message...
> 
> > 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 +-
> 
> > +++ 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);
> 
> ...does not match code. (Code is correct).
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

You're right. Thank you.
SeokYeon Hwang Nov. 12, 2014, 7:09 a.m. UTC | #8 
> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Monday, November 10, 2014 10:21 PM
> To: SeokYeon Hwang
> Cc: kongjianjun@gmail.com; paolo.bonzini@gmail.com; qemu-devel@nongnu.org;
> mreitz@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v3] error: passing a negative value to an
> os_errno is wrong
> 
> SeokYeon Hwang <syeon.hwang@samsung.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster [mailto:armbru@redhat.com]
> >> Sent: Monday, November 10, 2014 6:33 PM
> >> To: SeokYeon Hwang
> >> Cc: qemu-devel@nongnu.org; kongjianjun@gmail.com;
> >> paolo.bonzini@gmail.com; mreitz@redhat.com
> >> Subject: Re: [Qemu-devel] [PATCH v3] error: passing a negative value
> >> to an os_errno is wrong
> >>
> >> SeokYeon Hwang <syeon.hwang@samsung.com> writes:
> >>
> >> > 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..6c9d995
> >> > 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));
> >>
> >> The first hunk could still go into 2.2 as a bug fix.  The rest can't.
> >> You could post just the first hunk as "[PATCH for-2.2] pci: Don't
> >> pass negative errno to error_set_errno()", with my R-by.
> >>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >
> > Did you mean separate this patch into two patches ??
> > One is "PATCH v4" and the other is "PATCH for-2.2". Am I right??
> 
> Yes.
> 
> > And I have a question. (I don't know review / merge process well.)
> > What happens to the "reviewed" but "not bug-fix" patch during "feature
> > freeze" time ??
> 
> They get applied when the next development cycle opens.  If they don't,
> you have to remind the maintainer(s).

I just posted separated two patches.
Thank you for your advice.
diff mbox

Patch

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..6c9d995 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));