* [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
@ 2019-05-14 13:15 ` Yury Kotov
0 siblings, 0 replies; 12+ messages in thread
From: Yury Kotov @ 2019-05-14 13:15 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, armbru
monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
a bug with removing fd from fdset which id >= 2^32.
So, fix return types for these function.
Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
include/monitor/monitor.h | 2 +-
monitor.c | 4 ++--
stubs/fdset.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index c1b40a9cac..2872621afd 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
id, int64_t fdset_id,
int monitor_fdset_get_fd(int64_t fdset_id, int flags);
int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
void monitor_fdset_dup_fd_remove(int dup_fd);
-int monitor_fdset_dup_fd_find(int dup_fd);
+int64_t monitor_fdset_dup_fd_find(int dup_fd);
=20
void monitor_vfprintf(FILE *stream,
const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
diff --git a/monitor.c b/monitor.c
index 4807bbe811..50e6e820d6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2585,7 +2585,7 @@ err:
return -1;
}
=20
-static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
+static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
{
MonFdset *mon_fdset;
MonFdsetFd *mon_fdset_fd_dup;
@@ -2613,7 +2613,7 @@ err:
return -1;
}
=20
-int monitor_fdset_dup_fd_find(int dup_fd)
+int64_t monitor_fdset_dup_fd_find(int dup_fd)
{
return monitor_fdset_dup_fd_find_remove(dup_fd, false);
}
diff --git a/stubs/fdset.c b/stubs/fdset.c
index 4f3edf2ea4..a1b8f41f62 100644
--- a/stubs/fdset.c
+++ b/stubs/fdset.c
@@ -7,7 +7,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd=
)
return -1;
}
=20
-int monitor_fdset_dup_fd_find(int dup_fd)
+int64_t monitor_fdset_dup_fd_find(int dup_fd)
{
return -1;
}
--=20
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
2019-05-14 13:15 ` [Qemu-devel] " Yury Kotov
@ 2019-05-14 14:00 ` Markus Armbruster
-1 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2019-05-14 14:00 UTC (permalink / raw)
To: Yury Kotov; +Cc: qemu-devel, qemu-trivial, armbru
Yury Kotov <yury-kotov@yandex-team.ru> writes:
> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
Grammar nits:
s/returns/return/
s/Downcast/Downcasting/
> a bug with removing fd from fdset which id >= 2^32.
s/which/with/
> So, fix return types for these function.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
If I feed your message to git-am, I get
Applying: monitor: Fix return type of monitor_fdset_dup_fd_find
error: corrupt patch at line 12
Patch failed at 0001 monitor: Fix return type of monitor_fdset_dup_fd_find
[...]
Did you use git-send-email?
> include/monitor/monitor.h | 2 +-
> monitor.c | 4 ++--
> stubs/fdset.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index c1b40a9cac..2872621afd 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
> id, int64_t fdset_id,
> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> void monitor_fdset_dup_fd_remove(int dup_fd);
> -int monitor_fdset_dup_fd_find(int dup_fd);
> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
> =20
Looks mime-damaged.
> void monitor_vfprintf(FILE *stream,
> const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
> diff --git a/monitor.c b/monitor.c
> index 4807bbe811..50e6e820d6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2585,7 +2585,7 @@ err:
> return -1;
> }
> =20
> -static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> +static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> {
> MonFdset *mon_fdset;
> MonFdsetFd *mon_fdset_fd_dup;
> @@ -2613,7 +2613,7 @@ err:
> return -1;
> }
> =20
> -int monitor_fdset_dup_fd_find(int dup_fd)
> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
> {
> return monitor_fdset_dup_fd_find_remove(dup_fd, false);
> }
> diff --git a/stubs/fdset.c b/stubs/fdset.c
> index 4f3edf2ea4..a1b8f41f62 100644
> --- a/stubs/fdset.c
> +++ b/stubs/fdset.c
> @@ -7,7 +7,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd=
> )
> return -1;
> }
> =20
> -int monitor_fdset_dup_fd_find(int dup_fd)
> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
> {
> return -1;
> }
> --=20
> 2.21.0
The patch is complete because:
* monitor_fdset_dup_fd_find_remove() is used only by
monitor_fdset_dup_fd_find(), which you fix as well, and
monitor_fdset_dup_fd_remove(), which ignores the return value.
* monitor_fdset_dup_fd_find() is used only by qemu_close(), which stores
the return value in an int64_t.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
@ 2019-05-14 14:00 ` Markus Armbruster
0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2019-05-14 14:00 UTC (permalink / raw)
To: Yury Kotov; +Cc: qemu-trivial, qemu-devel, armbru
Yury Kotov <yury-kotov@yandex-team.ru> writes:
> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
Grammar nits:
s/returns/return/
s/Downcast/Downcasting/
> a bug with removing fd from fdset which id >= 2^32.
s/which/with/
> So, fix return types for these function.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
If I feed your message to git-am, I get
Applying: monitor: Fix return type of monitor_fdset_dup_fd_find
error: corrupt patch at line 12
Patch failed at 0001 monitor: Fix return type of monitor_fdset_dup_fd_find
[...]
Did you use git-send-email?
> include/monitor/monitor.h | 2 +-
> monitor.c | 4 ++--
> stubs/fdset.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index c1b40a9cac..2872621afd 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
> id, int64_t fdset_id,
> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> void monitor_fdset_dup_fd_remove(int dup_fd);
> -int monitor_fdset_dup_fd_find(int dup_fd);
> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
> =20
Looks mime-damaged.
> void monitor_vfprintf(FILE *stream,
> const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
> diff --git a/monitor.c b/monitor.c
> index 4807bbe811..50e6e820d6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2585,7 +2585,7 @@ err:
> return -1;
> }
> =20
> -static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> +static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> {
> MonFdset *mon_fdset;
> MonFdsetFd *mon_fdset_fd_dup;
> @@ -2613,7 +2613,7 @@ err:
> return -1;
> }
> =20
> -int monitor_fdset_dup_fd_find(int dup_fd)
> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
> {
> return monitor_fdset_dup_fd_find_remove(dup_fd, false);
> }
> diff --git a/stubs/fdset.c b/stubs/fdset.c
> index 4f3edf2ea4..a1b8f41f62 100644
> --- a/stubs/fdset.c
> +++ b/stubs/fdset.c
> @@ -7,7 +7,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd=
> )
> return -1;
> }
> =20
> -int monitor_fdset_dup_fd_find(int dup_fd)
> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
> {
> return -1;
> }
> --=20
> 2.21.0
The patch is complete because:
* monitor_fdset_dup_fd_find_remove() is used only by
monitor_fdset_dup_fd_find(), which you fix as well, and
monitor_fdset_dup_fd_remove(), which ignores the return value.
* monitor_fdset_dup_fd_find() is used only by qemu_close(), which stores
the return value in an int64_t.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
2019-05-14 14:00 ` Markus Armbruster
@ 2019-05-14 14:59 ` Yury Kotov
-1 siblings, 0 replies; 12+ messages in thread
From: Yury Kotov @ 2019-05-14 14:59 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-trivial@nongnu.org, qemu-devel@nongnu.org, Eric Blake
14.05.2019, 17:01, "Markus Armbruster" <armbru@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> writes:
>
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>
> Grammar nits:
>
> s/returns/return/
> s/Downcast/Downcasting/
>
>> a bug with removing fd from fdset which id >= 2^32.
>
> s/which/with/
>
>> So, fix return types for these function.
>>
>> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>> ---
>
> If I feed your message to git-am, I get
>
> Applying: monitor: Fix return type of monitor_fdset_dup_fd_find
> error: corrupt patch at line 12
> Patch failed at 0001 monitor: Fix return type of monitor_fdset_dup_fd_find
> [...]
>
> Did you use git-send-email?
>
>> include/monitor/monitor.h | 2 +-
>> monitor.c | 4 ++--
>> stubs/fdset.c | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> index c1b40a9cac..2872621afd 100644
>> --- a/include/monitor/monitor.h
>> +++ b/include/monitor/monitor.h
>> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
>> id, int64_t fdset_id,
>> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
>> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>> void monitor_fdset_dup_fd_remove(int dup_fd);
>> -int monitor_fdset_dup_fd_find(int dup_fd);
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
>> =20
>
> Looks mime-damaged.
>
>> void monitor_vfprintf(FILE *stream,
>> const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
>> diff --git a/monitor.c b/monitor.c
>> index 4807bbe811..50e6e820d6 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2585,7 +2585,7 @@ err:
>> return -1;
>> }
>> =20
>> -static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>> +static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>> {
>> MonFdset *mon_fdset;
>> MonFdsetFd *mon_fdset_fd_dup;
>> @@ -2613,7 +2613,7 @@ err:
>> return -1;
>> }
>> =20
>> -int monitor_fdset_dup_fd_find(int dup_fd)
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
>> {
>> return monitor_fdset_dup_fd_find_remove(dup_fd, false);
>> }
>> diff --git a/stubs/fdset.c b/stubs/fdset.c
>> index 4f3edf2ea4..a1b8f41f62 100644
>> --- a/stubs/fdset.c
>> +++ b/stubs/fdset.c
>> @@ -7,7 +7,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd=
>> )
>> return -1;
>> }
>> =20
>> -int monitor_fdset_dup_fd_find(int dup_fd)
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
>> {
>> return -1;
>> }
>> --=20
>> 2.21.0
>
> The patch is complete because:
>
> * monitor_fdset_dup_fd_find_remove() is used only by
> monitor_fdset_dup_fd_find(), which you fix as well, and
> monitor_fdset_dup_fd_remove(), which ignores the return value.
>
> * monitor_fdset_dup_fd_find() is used only by qemu_close(), which stores
> the return value in an int64_t.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Sorry for corrupted patch, will be more careful next time.
Because of Eric's suggestion, v2 will be completely different, so I think it is
more correct not to keep the "Reviewed-by".
Regards,
Yury
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [Qemu-trivial] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
@ 2019-05-14 14:59 ` Yury Kotov
0 siblings, 0 replies; 12+ messages in thread
From: Yury Kotov @ 2019-05-14 14:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-trivial@nongnu.org, qemu-devel@nongnu.org
14.05.2019, 17:01, "Markus Armbruster" <armbru@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> writes:
>
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>
> Grammar nits:
>
> s/returns/return/
> s/Downcast/Downcasting/
>
>> a bug with removing fd from fdset which id >= 2^32.
>
> s/which/with/
>
>> So, fix return types for these function.
>>
>> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>> ---
>
> If I feed your message to git-am, I get
>
> Applying: monitor: Fix return type of monitor_fdset_dup_fd_find
> error: corrupt patch at line 12
> Patch failed at 0001 monitor: Fix return type of monitor_fdset_dup_fd_find
> [...]
>
> Did you use git-send-email?
>
>> include/monitor/monitor.h | 2 +-
>> monitor.c | 4 ++--
>> stubs/fdset.c | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> index c1b40a9cac..2872621afd 100644
>> --- a/include/monitor/monitor.h
>> +++ b/include/monitor/monitor.h
>> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
>> id, int64_t fdset_id,
>> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
>> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>> void monitor_fdset_dup_fd_remove(int dup_fd);
>> -int monitor_fdset_dup_fd_find(int dup_fd);
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
>> =20
>
> Looks mime-damaged.
>
>> void monitor_vfprintf(FILE *stream,
>> const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
>> diff --git a/monitor.c b/monitor.c
>> index 4807bbe811..50e6e820d6 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2585,7 +2585,7 @@ err:
>> return -1;
>> }
>> =20
>> -static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>> +static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>> {
>> MonFdset *mon_fdset;
>> MonFdsetFd *mon_fdset_fd_dup;
>> @@ -2613,7 +2613,7 @@ err:
>> return -1;
>> }
>> =20
>> -int monitor_fdset_dup_fd_find(int dup_fd)
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
>> {
>> return monitor_fdset_dup_fd_find_remove(dup_fd, false);
>> }
>> diff --git a/stubs/fdset.c b/stubs/fdset.c
>> index 4f3edf2ea4..a1b8f41f62 100644
>> --- a/stubs/fdset.c
>> +++ b/stubs/fdset.c
>> @@ -7,7 +7,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd=
>> )
>> return -1;
>> }
>> =20
>> -int monitor_fdset_dup_fd_find(int dup_fd)
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd)
>> {
>> return -1;
>> }
>> --=20
>> 2.21.0
>
> The patch is complete because:
>
> * monitor_fdset_dup_fd_find_remove() is used only by
> monitor_fdset_dup_fd_find(), which you fix as well, and
> monitor_fdset_dup_fd_remove(), which ignores the return value.
>
> * monitor_fdset_dup_fd_find() is used only by qemu_close(), which stores
> the return value in an int64_t.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Sorry for corrupted patch, will be more careful next time.
Because of Eric's suggestion, v2 will be completely different, so I think it is
more correct not to keep the "Reviewed-by".
Regards,
Yury
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
2019-05-14 13:15 ` [Qemu-devel] " Yury Kotov
@ 2019-05-14 14:05 ` Eric Blake
-1 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-05-14 14:05 UTC (permalink / raw)
To: Yury Kotov, qemu-devel; +Cc: qemu-trivial, armbru
[-- Attachment #1.1: Type: text/plain, Size: 1502 bytes --]
On 5/14/19 8:15 AM, Yury Kotov wrote:
> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
> a bug with removing fd from fdset which id >= 2^32.
> So, fix return types for these function.
fd's cannot exceed 2^32. We should instead be fixing anything that uses
int64_t with an fd to be properly limited to 32 bits. That is, I think
the real problem is in qapi/misc.json:
{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
instead of 'fd':'int32'. For that matter, 'fdset-id' larger than 32
bits is unlikely to be useful (there's no reason to have more fdsets
than you can have possible fds to put in those sets).
NACK to this version, but a v2 that addresses the real problem is
worthwhile.
> +++ b/include/monitor/monitor.h
> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
> id, int64_t fdset_id,
> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> void monitor_fdset_dup_fd_remove(int dup_fd);
> -int monitor_fdset_dup_fd_find(int dup_fd);
> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
> =20
Your patch came through corrupted. You may want to double-check how you
are sending them, to ensure they are not mangled.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
@ 2019-05-14 14:05 ` Eric Blake
0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-05-14 14:05 UTC (permalink / raw)
To: Yury Kotov, qemu-devel; +Cc: qemu-trivial, armbru
[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]
On 5/14/19 8:15 AM, Yury Kotov wrote:
> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
> a bug with removing fd from fdset which id >= 2^32.
> So, fix return types for these function.
fd's cannot exceed 2^32. We should instead be fixing anything that uses
int64_t with an fd to be properly limited to 32 bits. That is, I think
the real problem is in qapi/misc.json:
{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
instead of 'fd':'int32'. For that matter, 'fdset-id' larger than 32
bits is unlikely to be useful (there's no reason to have more fdsets
than you can have possible fds to put in those sets).
NACK to this version, but a v2 that addresses the real problem is
worthwhile.
> +++ b/include/monitor/monitor.h
> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
> id, int64_t fdset_id,
> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> void monitor_fdset_dup_fd_remove(int dup_fd);
> -int monitor_fdset_dup_fd_find(int dup_fd);
> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
> =20
Your patch came through corrupted. You may want to double-check how you
are sending them, to ensure they are not mangled.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-trivial] [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
2019-05-14 14:05 ` Eric Blake
@ 2019-05-14 14:51 ` Yury Kotov
-1 siblings, 0 replies; 12+ messages in thread
From: Yury Kotov @ 2019-05-14 14:51 UTC (permalink / raw)
To: Eric Blake, qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org, armbru@redhat.com
14.05.2019, 17:05, "Eric Blake" <eblake@redhat.com>:
> On 5/14/19 8:15 AM, Yury Kotov wrote:
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>> a bug with removing fd from fdset which id >= 2^32.
>> So, fix return types for these function.
>
> fd's cannot exceed 2^32. We should instead be fixing anything that uses
> int64_t with an fd to be properly limited to 32 bits. That is, I think
> the real problem is in qapi/misc.json:
>
> { 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> instead of 'fd':'int32'. For that matter, 'fdset-id' larger than 32
> bits is unlikely to be useful (there's no reason to have more fdsets
> than you can have possible fds to put in those sets).
>
> NACK to this version, but a v2 that addresses the real problem is
> worthwhile.
>
Ok, so I will change fdset_id type int64_t -> int everywhere and
int64_t -> int32_t for qmp commands. Right?
>> +++ b/include/monitor/monitor.h
>> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
>> id, int64_t fdset_id,
>> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
>> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>> void monitor_fdset_dup_fd_remove(int dup_fd);
>> -int monitor_fdset_dup_fd_find(int dup_fd);
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
>> =20
>
> Your patch came through corrupted. You may want to double-check how you
> are sending them, to ensure they are not mangled.
>
Omg, sorry. I just copy-pasted my previous patch from mail client in raw format
and I did not notice the escape characters, because of which the patch is
incorrect.
Regards,
Yury
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
@ 2019-05-14 14:51 ` Yury Kotov
0 siblings, 0 replies; 12+ messages in thread
From: Yury Kotov @ 2019-05-14 14:51 UTC (permalink / raw)
To: Eric Blake, qemu-devel@nongnu.org
Cc: qemu-trivial@nongnu.org, armbru@redhat.com
14.05.2019, 17:05, "Eric Blake" <eblake@redhat.com>:
> On 5/14/19 8:15 AM, Yury Kotov wrote:
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>> a bug with removing fd from fdset which id >= 2^32.
>> So, fix return types for these function.
>
> fd's cannot exceed 2^32. We should instead be fixing anything that uses
> int64_t with an fd to be properly limited to 32 bits. That is, I think
> the real problem is in qapi/misc.json:
>
> { 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> instead of 'fd':'int32'. For that matter, 'fdset-id' larger than 32
> bits is unlikely to be useful (there's no reason to have more fdsets
> than you can have possible fds to put in those sets).
>
> NACK to this version, but a v2 that addresses the real problem is
> worthwhile.
>
Ok, so I will change fdset_id type int64_t -> int everywhere and
int64_t -> int32_t for qmp commands. Right?
>> +++ b/include/monitor/monitor.h
>> @@ -46,7 +46,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_=
>> id, int64_t fdset_id,
>> int monitor_fdset_get_fd(int64_t fdset_id, int flags);
>> int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>> void monitor_fdset_dup_fd_remove(int dup_fd);
>> -int monitor_fdset_dup_fd_find(int dup_fd);
>> +int64_t monitor_fdset_dup_fd_find(int dup_fd);
>> =20
>
> Your patch came through corrupted. You may want to double-check how you
> are sending them, to ensure they are not mangled.
>
Omg, sorry. I just copy-pasted my previous patch from mail client in raw format
and I did not notice the escape characters, because of which the patch is
incorrect.
Regards,
Yury
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
2019-05-14 14:05 ` Eric Blake
@ 2019-05-14 17:01 ` Markus Armbruster
-1 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2019-05-14 17:01 UTC (permalink / raw)
To: Eric Blake; +Cc: Yury Kotov, qemu-devel, qemu-trivial
Eric Blake <eblake@redhat.com> writes:
> On 5/14/19 8:15 AM, Yury Kotov wrote:
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>> a bug with removing fd from fdset which id >= 2^32.
>> So, fix return types for these function.
>
> fd's cannot exceed 2^32. We should instead be fixing anything that uses
> int64_t with an fd to be properly limited to 32 bits. That is, I think
> the real problem is in qapi/misc.json:
>
> { 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> instead of 'fd':'int32'.
This is actually not related to the patch. It doesn't touch
file-descriptors at all, only fdset IDs.
But let's discuss file descriptors briefly.
File descriptors are plain int. There is no QAPI type corresponding to
plain int.
I guess plain int is 32 bits wide on all hosts we support. Narrower int
(permitted by the standard) wouldn't fly with QEMU. Wider int should,
and are theoretically possible.
I'm not sure we want to change the QAPI schema.
> For that matter, 'fdset-id' larger than 32
> bits is unlikely to be useful (there's no reason to have more fdsets
> than you can have possible fds to put in those sets).
Even if we had wider file descriptors: a billion fdsets should be enough
for anyone.
> NACK to this version, but a v2 that addresses the real problem is
> worthwhile.
What exactly is wrong with the patch?
It changes the return value of monitor_fdset_dup_fd_find_remove() and
monitor_fdset_dup_fd_find() from int to int64_t. Both return an fdset
ID (a MonFdset member @id, of type int64_t) on success, -1 on error.
The change removes a truncation from int64_t to int in
monitor_fdset_dup_fd_find_remove(), and a widening from int to int64_t
in qemu_close().
I believe the patch is fine as is. Another patch that changes fdset IDs
from int64_t to int32_t would also be fine, but it would Require
tracking down all the places to change.
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH RESEND] monitor: Fix return type of monitor_fdset_dup_fd_find
@ 2019-05-14 17:01 ` Markus Armbruster
0 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2019-05-14 17:01 UTC (permalink / raw)
To: Eric Blake; +Cc: Yury Kotov, qemu-trivial, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 5/14/19 8:15 AM, Yury Kotov wrote:
>> monitor_fdset_dup_fd_find_remove() and monitor_fdset_dup_fd_find()
>> returns mon_fdset->id which is int64_t. Downcast from int64_t to int leads to
>> a bug with removing fd from fdset which id >= 2^32.
>> So, fix return types for these function.
>
> fd's cannot exceed 2^32. We should instead be fixing anything that uses
> int64_t with an fd to be properly limited to 32 bits. That is, I think
> the real problem is in qapi/misc.json:
>
> { 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> instead of 'fd':'int32'.
This is actually not related to the patch. It doesn't touch
file-descriptors at all, only fdset IDs.
But let's discuss file descriptors briefly.
File descriptors are plain int. There is no QAPI type corresponding to
plain int.
I guess plain int is 32 bits wide on all hosts we support. Narrower int
(permitted by the standard) wouldn't fly with QEMU. Wider int should,
and are theoretically possible.
I'm not sure we want to change the QAPI schema.
> For that matter, 'fdset-id' larger than 32
> bits is unlikely to be useful (there's no reason to have more fdsets
> than you can have possible fds to put in those sets).
Even if we had wider file descriptors: a billion fdsets should be enough
for anyone.
> NACK to this version, but a v2 that addresses the real problem is
> worthwhile.
What exactly is wrong with the patch?
It changes the return value of monitor_fdset_dup_fd_find_remove() and
monitor_fdset_dup_fd_find() from int to int64_t. Both return an fdset
ID (a MonFdset member @id, of type int64_t) on success, -1 on error.
The change removes a truncation from int64_t to int in
monitor_fdset_dup_fd_find_remove(), and a widening from int to int64_t
in qemu_close().
I believe the patch is fine as is. Another patch that changes fdset IDs
from int64_t to int32_t would also be fine, but it would Require
tracking down all the places to change.
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread