qemu-char: check errno together with ret < 0

[Qemu-devel] [PATCH] qemu-char: check errno together with ret < 0

Posted by xinhua.Cao 7 years, 7 months ago

In the tcp_chr_write function, we checked errno,
but errno was not reset before a read or write operation.
Therefore, this check of errno's actions is often
incorrect after EAGAIN has occurred.
we need check errno together with ret < 0.

Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
---
 chardev/char-socket.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 17519ec..efbad6e 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -134,8 +134,11 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
                                         s->write_msgfds,
                                         s->write_msgfds_num);
 
-        /* free the written msgfds in any cases other than errno==EAGAIN */
-        if (EAGAIN != errno && s->write_msgfds_num) {
+        /* free the written msgfds in any cases
+         * other than ret < 0 && errno == EAGAIN
+         */
+        if (!(ret < 0 && EAGAIN == errno)
+            && s->write_msgfds_num) {
             g_free(s->write_msgfds);
             s->write_msgfds = 0;
             s->write_msgfds_num = 0;
-- 
2.8.3

Re: [Qemu-devel] [PATCH] qemu-char: check errno together with ret < 0

Posted by Daniel P. Berrangé 7 years, 7 months ago

On Wed, Jul 04, 2018 at 11:36:42AM +0800, xinhua.Cao wrote:
> In the tcp_chr_write function, we checked errno,
> but errno was not reset before a read or write operation.
> Therefore, this check of errno's actions is often
> incorrect after EAGAIN has occurred.
> we need check errno together with ret < 0.
> 
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
> ---
>  chardev/char-socket.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 17519ec..efbad6e 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,11 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
>  
> -        /* free the written msgfds in any cases other than errno==EAGAIN */
> -        if (EAGAIN != errno && s->write_msgfds_num) {
> +        /* free the written msgfds in any cases
> +         * other than ret < 0 && errno == EAGAIN
> +         */
> +        if (!(ret < 0 && EAGAIN == errno)
> +            && s->write_msgfds_num) {
>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] qemu-char: check errno together with ret < 0

Posted by Marc-André Lureau 7 years, 7 months ago

On Wed, Jul 4, 2018 at 5:36 AM, xinhua.Cao <caoxinhua@huawei.com> wrote:
> In the tcp_chr_write function, we checked errno,
> but errno was not reset before a read or write operation.
> Therefore, this check of errno's actions is often
> incorrect after EAGAIN has occurred.
> we need check errno together with ret < 0.
>
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>


Can you point out this is fixing commit
9fc53a10f81d3a9027b23fa810147d21be29e614 in commit message?

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 17519ec..efbad6e 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,11 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
>
> -        /* free the written msgfds in any cases other than errno==EAGAIN */
> -        if (EAGAIN != errno && s->write_msgfds_num) {
> +        /* free the written msgfds in any cases
> +         * other than ret < 0 && errno == EAGAIN
> +         */
> +        if (!(ret < 0 && EAGAIN == errno)
> +            && s->write_msgfds_num) {
>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;
> --
> 2.8.3
>
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] qemu-char: check errno together with ret < 0

Posted by Paolo Bonzini 7 years, 7 months ago

On 04/07/2018 11:36, Marc-André Lureau wrote:
> On Wed, Jul 4, 2018 at 5:36 AM, xinhua.Cao <caoxinhua@huawei.com> wrote:
>> In the tcp_chr_write function, we checked errno,
>> but errno was not reset before a read or write operation.
>> Therefore, this check of errno's actions is often
>> incorrect after EAGAIN has occurred.
>> we need check errno together with ret < 0.
>>
>> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
> 
> 
> Can you point out this is fixing commit
> 9fc53a10f81d3a9027b23fa810147d21be29e614 in commit message?

Ok, done.

Thanks,

Paolo

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
>> ---
>>  chardev/char-socket.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index 17519ec..efbad6e 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -134,8 +134,11 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>>                                          s->write_msgfds,
>>                                          s->write_msgfds_num);
>>
>> -        /* free the written msgfds in any cases other than errno==EAGAIN */
>> -        if (EAGAIN != errno && s->write_msgfds_num) {
>> +        /* free the written msgfds in any cases
>> +         * other than ret < 0 && errno == EAGAIN
>> +         */
>> +        if (!(ret < 0 && EAGAIN == errno)
>> +            && s->write_msgfds_num) {
>>              g_free(s->write_msgfds);
>>              s->write_msgfds = 0;
>>              s->write_msgfds_num = 0;
>> --
>> 2.8.3
>>
>>
>>
> 
> 
>

Re: [Qemu-devel] [PATCH] qemu-char: check errno together with ret < 0

Posted by Markus Armbruster 7 years, 7 months ago

xinhua.Cao <caoxinhua@huawei.com> writes:

> In the tcp_chr_write function, we checked errno,
> but errno was not reset before a read or write operation.
> Therefore, this check of errno's actions is often
> incorrect after EAGAIN has occurred.
> we need check errno together with ret < 0.
>
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
> ---
>  chardev/char-socket.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 17519ec..efbad6e 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,11 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
>  
> -        /* free the written msgfds in any cases other than errno==EAGAIN */
> -        if (EAGAIN != errno && s->write_msgfds_num) {
> +        /* free the written msgfds in any cases
> +         * other than ret < 0 && errno == EAGAIN
> +         */
> +        if (!(ret < 0 && EAGAIN == errno)
> +            && s->write_msgfds_num) {
>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;

The comment feels pretty worthless to me.