* [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
@ 2018-07-02 0:49 xinhua.Cao
2018-07-02 1:42 ` Philippe Mathieu-Daudé
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: xinhua.Cao @ 2018-07-02 0:49 UTC (permalink / raw)
To: pbonzini, marcandre.lureau, eblake, anton.nefedov, vsementsov
Cc: weidong.huang, weifuqiang, king.wang, arei.gonglei, liuyongan,
qemu-devel, xinhua.Cao
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 reset errno before reading and writing to
ensure the correctness of errno's judgment
Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
---
chardev/char-fe.c | 1 +
chardev/char.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b1f228e..d96ca6f 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -69,6 +69,7 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
while (offset < len) {
retry:
+ errno = 0;
res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
len - offset);
if (res == -1 && errno == EAGAIN) {
diff --git a/chardev/char.c b/chardev/char.c
index 76d866e..3387442 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -85,6 +85,7 @@ static void qemu_chr_write_log(Chardev *s, const uint8_t *buf, size_t len)
while (done < len) {
retry:
+ errno = 0;
ret = write(s->logfd, buf + done, len - done);
if (ret == -1 && errno == EAGAIN) {
g_usleep(100);
@@ -109,6 +110,7 @@ static int qemu_chr_write_buffer(Chardev *s,
qemu_mutex_lock(&s->chr_write_lock);
while (*offset < len) {
retry:
+ errno = 0;
res = cc->chr_write(s, buf + *offset, len - *offset);
if (res < 0 && errno == EAGAIN && write_all) {
g_usleep(100);
--
2.8.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
2018-07-02 0:49 [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action xinhua.Cao
@ 2018-07-02 1:42 ` Philippe Mathieu-Daudé
2018-07-02 8:46 ` Paolo Bonzini
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-07-02 1:42 UTC (permalink / raw)
To: xinhua.Cao, pbonzini, marcandre.lureau, eblake, anton.nefedov,
vsementsov
Cc: weidong.huang, weifuqiang, qemu-devel, king.wang, liuyongan,
arei.gonglei
On 07/01/2018 09:49 PM, 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 reset errno before reading and writing to
> ensure the correctness of errno's judgment
>
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> chardev/char-fe.c | 1 +
> chardev/char.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index b1f228e..d96ca6f 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -69,6 +69,7 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
>
> while (offset < len) {
> retry:
> + errno = 0;
> res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
> len - offset);
> if (res == -1 && errno == EAGAIN) {
> diff --git a/chardev/char.c b/chardev/char.c
> index 76d866e..3387442 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -85,6 +85,7 @@ static void qemu_chr_write_log(Chardev *s, const uint8_t *buf, size_t len)
>
> while (done < len) {
> retry:
> + errno = 0;
> ret = write(s->logfd, buf + done, len - done);
> if (ret == -1 && errno == EAGAIN) {
> g_usleep(100);
> @@ -109,6 +110,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> qemu_mutex_lock(&s->chr_write_lock);
> while (*offset < len) {
> retry:
> + errno = 0;
> res = cc->chr_write(s, buf + *offset, len - *offset);
> if (res < 0 && errno == EAGAIN && write_all) {
> g_usleep(100);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
2018-07-02 0:49 [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action xinhua.Cao
2018-07-02 1:42 ` Philippe Mathieu-Daudé
@ 2018-07-02 8:46 ` Paolo Bonzini
2018-07-04 4:12 ` xinhua.Cao
2018-07-02 11:57 ` Eric Blake
2018-07-02 15:01 ` Stefan Hajnoczi
3 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2018-07-02 8:46 UTC (permalink / raw)
To: xinhua.Cao, marcandre.lureau, eblake, anton.nefedov, vsementsov
Cc: weidong.huang, weifuqiang, qemu-devel, king.wang, liuyongan,
arei.gonglei
On 02/07/2018 02:49, 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 reset errno before reading and writing to
> ensure the correctness of errno's judgment
You should explain why this is a problem, because all the places you
modified are checking that the read or write has returned -1. In that
case, errno must have been modified and it is unnecessary to write 0.
Thanks,
Paolo
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
> ---
> chardev/char-fe.c | 1 +
> chardev/char.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index b1f228e..d96ca6f 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -69,6 +69,7 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
>
> while (offset < len) {
> retry:
> + errno = 0;
> res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
> len - offset);
> if (res == -1 && errno == EAGAIN) {
> diff --git a/chardev/char.c b/chardev/char.c
> index 76d866e..3387442 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -85,6 +85,7 @@ static void qemu_chr_write_log(Chardev *s, const uint8_t *buf, size_t len)
>
> while (done < len) {
> retry:
> + errno = 0;
> ret = write(s->logfd, buf + done, len - done);
> if (ret == -1 && errno == EAGAIN) {
> g_usleep(100);
> @@ -109,6 +110,7 @@ static int qemu_chr_write_buffer(Chardev *s,
> qemu_mutex_lock(&s->chr_write_lock);
> while (*offset < len) {
> retry:
> + errno = 0;
> res = cc->chr_write(s, buf + *offset, len - *offset);
> if (res < 0 && errno == EAGAIN && write_all) {
> g_usleep(100);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
2018-07-02 0:49 [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action xinhua.Cao
2018-07-02 1:42 ` Philippe Mathieu-Daudé
2018-07-02 8:46 ` Paolo Bonzini
@ 2018-07-02 11:57 ` Eric Blake
2018-07-02 15:01 ` Stefan Hajnoczi
3 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2018-07-02 11:57 UTC (permalink / raw)
To: xinhua.Cao, pbonzini, marcandre.lureau, anton.nefedov, vsementsov
Cc: weidong.huang, weifuqiang, king.wang, arei.gonglei, liuyongan,
qemu-devel
On 07/01/2018 07:49 PM, 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 reset errno before reading and writing to
> ensure the correctness of errno's judgment
>
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
> ---
> chardev/char-fe.c | 1 +
> chardev/char.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index b1f228e..d96ca6f 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -69,6 +69,7 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
>
> while (offset < len) {
> retry:
> + errno = 0;
> res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
> len - offset);
> if (res == -1 && errno == EAGAIN) {
NACK. read() is guaranteed to have set errno if res is -1, and we don't
check errno unless we first checked that res was -1. Thus, the EAGAIN
check is not confused by a stale errno value.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
2018-07-02 0:49 [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action xinhua.Cao
` (2 preceding siblings ...)
2018-07-02 11:57 ` Eric Blake
@ 2018-07-02 15:01 ` Stefan Hajnoczi
2018-07-04 2:10 ` xinhua.Cao
3 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2018-07-02 15:01 UTC (permalink / raw)
To: xinhua.Cao
Cc: pbonzini, marcandre.lureau, eblake, anton.nefedov, vsementsov,
weidong.huang, weifuqiang, qemu-devel, king.wang, liuyongan,
arei.gonglei
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
On Mon, Jul 02, 2018 at 08:49:10AM +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 reset errno before reading and writing to
> ensure the correctness of errno's judgment
>
> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
> ---
> chardev/char-fe.c | 1 +
> chardev/char.c | 2 ++
> 2 files changed, 3 insertions(+)
The C11x standard "7.4 Errors <errno.h>" says that errno "is never set
to zero by any library function". Please follow the errno usage
convention described in the C standard.
Instead of making functions called by tcp_chr_write() clear errno,
please fix tcp_chr_write() so that errno is only accessed when ret < 0.
Thanks,
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
2018-07-02 15:01 ` Stefan Hajnoczi
@ 2018-07-04 2:10 ` xinhua.Cao
0 siblings, 0 replies; 7+ messages in thread
From: xinhua.Cao @ 2018-07-04 2:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: pbonzini, marcandre.lureau, eblake, anton.nefedov, vsementsov,
weidong.huang, weifuqiang, qemu-devel, king.wang, liuyongan,
arei.gonglei
在 2018/7/2 23:01, Stefan Hajnoczi 写道:
> On Mon, Jul 02, 2018 at 08:49:10AM +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 reset errno before reading and writing to
>> ensure the correctness of errno's judgment
>>
>> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
>> ---
>> chardev/char-fe.c | 1 +
>> chardev/char.c | 2 ++
>> 2 files changed, 3 insertions(+)
> The C11x standard "7.4 Errors <errno.h>" says that errno "is never set
> to zero by any library function". Please follow the errno usage
> convention described in the C standard.
yes, I know that errno must be used after a negative return value.
But if you check the return value in tcp_chr_write,
the logic is a bit more complicated. So I reset the errno in the outer
layer.
I will check the return value at tcp_chr_write at v2 patch.
> Instead of making functions called by tcp_chr_write() clear errno,
> please fix tcp_chr_write() so that errno is only accessed when ret < 0.
>
> Thanks,
> Stefan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action
2018-07-02 8:46 ` Paolo Bonzini
@ 2018-07-04 4:12 ` xinhua.Cao
0 siblings, 0 replies; 7+ messages in thread
From: xinhua.Cao @ 2018-07-04 4:12 UTC (permalink / raw)
To: Paolo Bonzini, marcandre.lureau, eblake, anton.nefedov, vsementsov
Cc: weidong.huang, weifuqiang, qemu-devel, king.wang, liuyongan,
arei.gonglei
在 2018/7/2 16:46, Paolo Bonzini 写道:
> On 02/07/2018 02:49, 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 reset errno before reading and writing to
>> ensure the correctness of errno's judgment
> You should explain why this is a problem, because all the places you
> modified are checking that the read or write has returned -1. In that
> case, errno must have been modified and it is unnecessary to write 0.
>
> Thanks,
>
> Paolo
We found this problem on qemu-2.6. At that time,
we backport the patch 9fc53a10 to qemu-2.6 and
found that when the virtual machine was started,
the fds of the ovs process increased a lot.
we check tcp_chr_write function, it is found
that errno is not reset. Therefore, when errno is
set to EAGAIN, write_msgfds will not be free subsequently.
In the qemu-2.6 version, another free write_msgfds is in vhost_user_write.
Vhost_user_write in qemu-2.6 check fd_num before calling
qemu_chr_fe_set_msgfds.
fd_num is 0 in many cases, so it won't be cleaned up here.
There have been a lot of cases of sending fds to ovs.
Thanks,
xinhua.Cao
>
>> Signed-off-by: xinhua.Cao <caoxinhua@huawei.com>
>> ---
>> chardev/char-fe.c | 1 +
>> chardev/char.c | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
>> index b1f228e..d96ca6f 100644
>> --- a/chardev/char-fe.c
>> +++ b/chardev/char-fe.c
>> @@ -69,6 +69,7 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
>>
>> while (offset < len) {
>> retry:
>> + errno = 0;
>> res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
>> len - offset);
>> if (res == -1 && errno == EAGAIN) {
>> diff --git a/chardev/char.c b/chardev/char.c
>> index 76d866e..3387442 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -85,6 +85,7 @@ static void qemu_chr_write_log(Chardev *s, const uint8_t *buf, size_t len)
>>
>> while (done < len) {
>> retry:
>> + errno = 0;
>> ret = write(s->logfd, buf + done, len - done);
>> if (ret == -1 && errno == EAGAIN) {
>> g_usleep(100);
>> @@ -109,6 +110,7 @@ static int qemu_chr_write_buffer(Chardev *s,
>> qemu_mutex_lock(&s->chr_write_lock);
>> while (*offset < len) {
>> retry:
>> + errno = 0;
>> res = cc->chr_write(s, buf + *offset, len - *offset);
>> if (res < 0 && errno == EAGAIN && write_all) {
>> g_usleep(100);
>>
>
> .
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-04 4:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-02 0:49 [Qemu-devel] [PATCH] qemu-char: reset errno before qemu char write or read action xinhua.Cao
2018-07-02 1:42 ` Philippe Mathieu-Daudé
2018-07-02 8:46 ` Paolo Bonzini
2018-07-04 4:12 ` xinhua.Cao
2018-07-02 11:57 ` Eric Blake
2018-07-02 15:01 ` Stefan Hajnoczi
2018-07-04 2:10 ` xinhua.Cao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.