[Qemu-devel] [PATCH v2] monitor: Fix fdset_id & fd types for corresponding QMP commands

Yury Kotov posted 1 patch 10 weeks ago
Test asan passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190514151848.5677-1-yury-kotov@yandex-team.ru
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>
include/monitor/monitor.h |  6 +++---
monitor.c                 | 18 +++++++++---------
qapi/misc.json            | 10 +++++-----
stubs/fdset.c             |  4 ++--
util/osdep.c              |  4 ++--
vl.c                      |  2 +-
6 files changed, 22 insertions(+), 22 deletions(-)

[Qemu-devel] [PATCH v2] monitor: Fix fdset_id & fd types for corresponding QMP commands

Posted by Yury Kotov 10 weeks ago
Now, fdset_id is int64, but in some places we work with it as int.
It seems that there is no sense to use int64 for fdset_id, so it's
better to fix inconsistency by changing fdset_id type to int and by
fixing the reference of corresponding QMP commands: add-fd, remove-fd,
query-fdsets.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 include/monitor/monitor.h |  6 +++---
 monitor.c                 | 18 +++++++++---------
 qapi/misc.json            | 10 +++++-----
 stubs/fdset.c             |  4 ++--
 util/osdep.c              |  4 ++--
 vl.c                      |  2 +-
 6 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 86656297f1..06f9b6c217 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -39,11 +39,11 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque);
 
-AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
+AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int fdset_id,
                                 bool has_opaque, const char *opaque,
                                 Error **errp);
-int monitor_fdset_get_fd(int64_t fdset_id, int flags);
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
+int monitor_fdset_get_fd(int fdset_id, int flags);
+int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd);
 void monitor_fdset_dup_fd_remove(int dup_fd);
 int monitor_fdset_dup_fd_find(int dup_fd);
 
diff --git a/monitor.c b/monitor.c
index bb48997913..b71ce816bc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -160,7 +160,7 @@ struct MonFdsetFd {
 /* file descriptor set containing fds passed via SCM_RIGHTS */
 typedef struct MonFdset MonFdset;
 struct MonFdset {
-    int64_t id;
+    int id;
     QLIST_HEAD(, MonFdsetFd) fds;
     QLIST_HEAD(, MonFdsetFd) dup_fds;
     QLIST_ENTRY(MonFdset) next;
@@ -2346,7 +2346,7 @@ static void monitor_fdsets_cleanup(void)
     qemu_mutex_unlock(&mon_fdsets_lock);
 }
 
-AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
+AddfdInfo *qmp_add_fd(bool has_fdset_id, int32_t fdset_id, bool has_opaque,
                       const char *opaque, Error **errp)
 {
     int fd;
@@ -2372,7 +2372,7 @@ error:
     return NULL;
 }
 
-void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
+void qmp_remove_fd(int32_t fdset_id, bool has_fd, int fd, Error **errp)
 {
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd;
@@ -2405,10 +2405,10 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
 error:
     qemu_mutex_unlock(&mon_fdsets_lock);
     if (has_fd) {
-        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64,
+        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32 ", fd:%" PRId32,
                  fdset_id, fd);
     } else {
-        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64, fdset_id);
+        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32, fdset_id);
     }
     error_setg(errp, QERR_FD_NOT_FOUND, fd_str);
 }
@@ -2454,7 +2454,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
     return fdset_list;
 }
 
-AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
+AddfdInfo *monitor_fdset_add_fd(int32_t fd, bool has_fdset_id, int32_t fdset_id,
                                 bool has_opaque, const char *opaque,
                                 Error **errp)
 {
@@ -2476,7 +2476,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
     }
 
     if (mon_fdset == NULL) {
-        int64_t fdset_id_prev = -1;
+        int fdset_id_prev = -1;
         MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
 
         if (has_fdset_id) {
@@ -2538,7 +2538,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
     return fdinfo;
 }
 
-int monitor_fdset_get_fd(int64_t fdset_id, int flags)
+int monitor_fdset_get_fd(int fdset_id, int flags)
 {
 #ifdef _WIN32
     return -ENOENT;
@@ -2576,7 +2576,7 @@ out:
 #endif
 }
 
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
+int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd)
 {
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;
diff --git a/qapi/misc.json b/qapi/misc.json
index 8b3ca4fdd3..b345e1458b 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -2179,7 +2179,7 @@
 #
 # Since: 1.2.0
 ##
-{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
+{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int32', 'fd': 'int32'} }
 
 ##
 # @add-fd:
@@ -2209,7 +2209,7 @@
 #
 ##
 { 'command': 'add-fd',
-  'data': { '*fdset-id': 'int',
+  'data': { '*fdset-id': 'int32',
             '*opaque': 'str' },
   'returns': 'AddfdInfo' }
 
@@ -2238,7 +2238,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'remove-fd', 'data': {'fdset-id': 'int', '*fd': 'int'} }
+{ 'command': 'remove-fd', 'data': {'fdset-id': 'int32', '*fd': 'int32'} }
 
 ##
 # @FdsetFdInfo:
@@ -2252,7 +2252,7 @@
 # Since: 1.2.0
 ##
 { 'struct': 'FdsetFdInfo',
-  'data': {'fd': 'int', '*opaque': 'str'} }
+  'data': {'fd': 'int32', '*opaque': 'str'} }
 
 ##
 # @FdsetInfo:
@@ -2266,7 +2266,7 @@
 # Since: 1.2.0
 ##
 { 'struct': 'FdsetInfo',
-  'data': {'fdset-id': 'int', 'fds': ['FdsetFdInfo']} }
+  'data': {'fdset-id': 'int32', 'fds': ['FdsetFdInfo']} }
 
 ##
 # @query-fdsets:
diff --git a/stubs/fdset.c b/stubs/fdset.c
index 4f3edf2ea4..1504624c19 100644
--- a/stubs/fdset.c
+++ b/stubs/fdset.c
@@ -2,7 +2,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
+int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd)
 {
     return -1;
 }
@@ -12,7 +12,7 @@ int monitor_fdset_dup_fd_find(int dup_fd)
     return -1;
 }
 
-int monitor_fdset_get_fd(int64_t fdset_id, int flags)
+int monitor_fdset_get_fd(int fdset_id, int flags)
 {
     return -ENOENT;
 }
diff --git a/util/osdep.c b/util/osdep.c
index 3f04326040..9e2d3768e0 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -292,7 +292,7 @@ int qemu_open(const char *name, int flags, ...)
 
     /* Attempt dup of fd from fd set */
     if (strstart(name, "/dev/fdset/", &fdset_id_str)) {
-        int64_t fdset_id;
+        int fdset_id;
         int fd, dupfd;
 
         fdset_id = qemu_parse_fdset(fdset_id_str);
@@ -352,7 +352,7 @@ int qemu_open(const char *name, int flags, ...)
 
 int qemu_close(int fd)
 {
-    int64_t fdset_id;
+    int fdset_id;
 
     /* Close fd that was dup'd from an fdset */
     fdset_id = monitor_fdset_dup_fd_find(fd);
diff --git a/vl.c b/vl.c
index b6709514c1..0f5622496c 100644
--- a/vl.c
+++ b/vl.c
@@ -1081,7 +1081,7 @@ bool defaults_enabled(void)
 static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
 {
     int fd, dupfd, flags;
-    int64_t fdset_id;
+    int fdset_id;
     const char *fd_opaque = NULL;
     AddfdInfo *fdinfo;
 
-- 
2.21.0


Re: [Qemu-devel] [PATCH v2] monitor: Fix fdset_id & fd types for corresponding QMP commands

Posted by Markus Armbruster 8 weeks ago
Yury Kotov <yury-kotov@yandex-team.ru> writes:

> Now, fdset_id is int64, but in some places we work with it as int.
> It seems that there is no sense to use int64 for fdset_id, so it's
> better to fix inconsistency by changing fdset_id type to int and by
> fixing the reference of corresponding QMP commands: add-fd, remove-fd,
> query-fdsets.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
>  include/monitor/monitor.h |  6 +++---
>  monitor.c                 | 18 +++++++++---------
>  qapi/misc.json            | 10 +++++-----
>  stubs/fdset.c             |  4 ++--
>  util/osdep.c              |  4 ++--
>  vl.c                      |  2 +-
>  6 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 86656297f1..06f9b6c217 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -39,11 +39,11 @@ void monitor_read_command(Monitor *mon, int show_prompt);
>  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>                            void *opaque);
>  
> -AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> +AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int fdset_id,
>                                  bool has_opaque, const char *opaque,
>                                  Error **errp);
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> +int monitor_fdset_get_fd(int fdset_id, int flags);
> +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd);
>  void monitor_fdset_dup_fd_remove(int dup_fd);
>  int monitor_fdset_dup_fd_find(int dup_fd);
>  
> diff --git a/monitor.c b/monitor.c
> index bb48997913..b71ce816bc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -160,7 +160,7 @@ struct MonFdsetFd {
>  /* file descriptor set containing fds passed via SCM_RIGHTS */
>  typedef struct MonFdset MonFdset;
>  struct MonFdset {
> -    int64_t id;
> +    int id;

In C, you use int instead of int64_t for fdset IDs.


>      QLIST_HEAD(, MonFdsetFd) fds;
>      QLIST_HEAD(, MonFdsetFd) dup_fds;
>      QLIST_ENTRY(MonFdset) next;
> @@ -2346,7 +2346,7 @@ static void monitor_fdsets_cleanup(void)
>      qemu_mutex_unlock(&mon_fdsets_lock);
>  }
>  
> -AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
> +AddfdInfo *qmp_add_fd(bool has_fdset_id, int32_t fdset_id, bool has_opaque,
>                        const char *opaque, Error **errp)
>  {
>      int fd;
> @@ -2372,7 +2372,7 @@ error:
>      return NULL;
>  }
>  
> -void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> +void qmp_remove_fd(int32_t fdset_id, bool has_fd, int fd, Error **errp)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd;
> @@ -2405,10 +2405,10 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
>  error:
>      qemu_mutex_unlock(&mon_fdsets_lock);
>      if (has_fd) {
> -        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64,
> +        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32 ", fd:%" PRId32,
>                   fdset_id, fd);
>      } else {
> -        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64, fdset_id);
> +        snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32, fdset_id);
>      }
>      error_setg(errp, QERR_FD_NOT_FOUND, fd_str);
>  }
> @@ -2454,7 +2454,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>      return fdset_list;
>  }
>  
> -AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> +AddfdInfo *monitor_fdset_add_fd(int32_t fd, bool has_fdset_id, int32_t fdset_id,
>                                  bool has_opaque, const char *opaque,
>                                  Error **errp)
>  {
> @@ -2476,7 +2476,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>      }
>  
>      if (mon_fdset == NULL) {
> -        int64_t fdset_id_prev = -1;
> +        int fdset_id_prev = -1;
>          MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
>  
>          if (has_fdset_id) {
> @@ -2538,7 +2538,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>      return fdinfo;
>  }
>  
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> +int monitor_fdset_get_fd(int fdset_id, int flags)
>  {
>  #ifdef _WIN32
>      return -ENOENT;
> @@ -2576,7 +2576,7 @@ out:
>  #endif
>  }
>  
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 8b3ca4fdd3..b345e1458b 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -2179,7 +2179,7 @@
>  #
>  # Since: 1.2.0
>  ##
> -{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> +{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int32', 'fd': 'int32'} }
>  
>  ##
>  # @add-fd:
> @@ -2209,7 +2209,7 @@
>  #
>  ##
>  { 'command': 'add-fd',
> -  'data': { '*fdset-id': 'int',
> +  'data': { '*fdset-id': 'int32',
>              '*opaque': 'str' },
>    'returns': 'AddfdInfo' }
>  
> @@ -2238,7 +2238,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'remove-fd', 'data': {'fdset-id': 'int', '*fd': 'int'} }
> +{ 'command': 'remove-fd', 'data': {'fdset-id': 'int32', '*fd': 'int32'} }
>  
>  ##
>  # @FdsetFdInfo:
> @@ -2252,7 +2252,7 @@
>  # Since: 1.2.0
>  ##
>  { 'struct': 'FdsetFdInfo',
> -  'data': {'fd': 'int', '*opaque': 'str'} }
> +  'data': {'fd': 'int32', '*opaque': 'str'} }
>  
>  ##
>  # @FdsetInfo:
> @@ -2266,7 +2266,7 @@
>  # Since: 1.2.0
>  ##
>  { 'struct': 'FdsetInfo',
> -  'data': {'fdset-id': 'int', 'fds': ['FdsetFdInfo']} }
> +  'data': {'fdset-id': 'int32', 'fds': ['FdsetFdInfo']} }
>  
>  ##
>  # @query-fdsets:

In the schema, you use QAPI type 'int32' instead of 'int'.

Before the patch, the two are consistent (except for the bugs you fixed
in v1 of this patch): QAPI 'int' is C int64_t.

After the patch, the two are inconsistent: QAPI 'int32' is C int32_t,
not int.  They're usually the same, but it unclean.

Two ways forward:

1. Revise this patch to use int32_t instead of int.

2. Revise v1 to address the few minor review comments I had.  Smaller
   patch, easier to review.

Your choice.  I'd choose 2.

[...]

Re: [Qemu-devel] [PATCH v2] monitor: Fix fdset_id & fd types for corresponding QMP commands

Posted by Yury Kotov 8 weeks ago
22.05.2019, 19:40, "Markus Armbruster" <armbru@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> writes:
>
>>  Now, fdset_id is int64, but in some places we work with it as int.
>>  It seems that there is no sense to use int64 for fdset_id, so it's
>>  better to fix inconsistency by changing fdset_id type to int and by
>>  fixing the reference of corresponding QMP commands: add-fd, remove-fd,
>>  query-fdsets.
>>
>>  Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>>  ---
>>   include/monitor/monitor.h | 6 +++---
>>   monitor.c | 18 +++++++++---------
>>   qapi/misc.json | 10 +++++-----
>>   stubs/fdset.c | 4 ++--
>>   util/osdep.c | 4 ++--
>>   vl.c | 2 +-
>>   6 files changed, 22 insertions(+), 22 deletions(-)
>>
>>  diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>>  index 86656297f1..06f9b6c217 100644
>>  --- a/include/monitor/monitor.h
>>  +++ b/include/monitor/monitor.h
>>  @@ -39,11 +39,11 @@ void monitor_read_command(Monitor *mon, int show_prompt);
>>   int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>>                             void *opaque);
>>
>>  -AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>>  +AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int fdset_id,
>>                                   bool has_opaque, const char *opaque,
>>                                   Error **errp);
>>  -int monitor_fdset_get_fd(int64_t fdset_id, int flags);
>>  -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
>>  +int monitor_fdset_get_fd(int fdset_id, int flags);
>>  +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd);
>>   void monitor_fdset_dup_fd_remove(int dup_fd);
>>   int monitor_fdset_dup_fd_find(int dup_fd);
>>
>>  diff --git a/monitor.c b/monitor.c
>>  index bb48997913..b71ce816bc 100644
>>  --- a/monitor.c
>>  +++ b/monitor.c
>>  @@ -160,7 +160,7 @@ struct MonFdsetFd {
>>   /* file descriptor set containing fds passed via SCM_RIGHTS */
>>   typedef struct MonFdset MonFdset;
>>   struct MonFdset {
>>  - int64_t id;
>>  + int id;
>
> In C, you use int instead of int64_t for fdset IDs.
>
>>       QLIST_HEAD(, MonFdsetFd) fds;
>>       QLIST_HEAD(, MonFdsetFd) dup_fds;
>>       QLIST_ENTRY(MonFdset) next;
>>  @@ -2346,7 +2346,7 @@ static void monitor_fdsets_cleanup(void)
>>       qemu_mutex_unlock(&mon_fdsets_lock);
>>   }
>>
>>  -AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
>>  +AddfdInfo *qmp_add_fd(bool has_fdset_id, int32_t fdset_id, bool has_opaque,
>>                         const char *opaque, Error **errp)
>>   {
>>       int fd;
>>  @@ -2372,7 +2372,7 @@ error:
>>       return NULL;
>>   }
>>
>>  -void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
>>  +void qmp_remove_fd(int32_t fdset_id, bool has_fd, int fd, Error **errp)
>>   {
>>       MonFdset *mon_fdset;
>>       MonFdsetFd *mon_fdset_fd;
>>  @@ -2405,10 +2405,10 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
>>   error:
>>       qemu_mutex_unlock(&mon_fdsets_lock);
>>       if (has_fd) {
>>  - snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64,
>>  + snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32 ", fd:%" PRId32,
>>                    fdset_id, fd);
>>       } else {
>>  - snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64, fdset_id);
>>  + snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32, fdset_id);
>>       }
>>       error_setg(errp, QERR_FD_NOT_FOUND, fd_str);
>>   }
>>  @@ -2454,7 +2454,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>>       return fdset_list;
>>   }
>>
>>  -AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>>  +AddfdInfo *monitor_fdset_add_fd(int32_t fd, bool has_fdset_id, int32_t fdset_id,
>>                                   bool has_opaque, const char *opaque,
>>                                   Error **errp)
>>   {
>>  @@ -2476,7 +2476,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>>       }
>>
>>       if (mon_fdset == NULL) {
>>  - int64_t fdset_id_prev = -1;
>>  + int fdset_id_prev = -1;
>>           MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
>>
>>           if (has_fdset_id) {
>>  @@ -2538,7 +2538,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>>       return fdinfo;
>>   }
>>
>>  -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>>  +int monitor_fdset_get_fd(int fdset_id, int flags)
>>   {
>>   #ifdef _WIN32
>>       return -ENOENT;
>>  @@ -2576,7 +2576,7 @@ out:
>>   #endif
>>   }
>>
>>  -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
>>  +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd)
>>   {
>>       MonFdset *mon_fdset;
>>       MonFdsetFd *mon_fdset_fd_dup;
>>  diff --git a/qapi/misc.json b/qapi/misc.json
>>  index 8b3ca4fdd3..b345e1458b 100644
>>  --- a/qapi/misc.json
>>  +++ b/qapi/misc.json
>>  @@ -2179,7 +2179,7 @@
>>   #
>>   # Since: 1.2.0
>>   ##
>>  -{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
>>  +{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int32', 'fd': 'int32'} }
>>
>>   ##
>>   # @add-fd:
>>  @@ -2209,7 +2209,7 @@
>>   #
>>   ##
>>   { 'command': 'add-fd',
>>  - 'data': { '*fdset-id': 'int',
>>  + 'data': { '*fdset-id': 'int32',
>>               '*opaque': 'str' },
>>     'returns': 'AddfdInfo' }
>>
>>  @@ -2238,7 +2238,7 @@
>>   # <- { "return": {} }
>>   #
>>   ##
>>  -{ 'command': 'remove-fd', 'data': {'fdset-id': 'int', '*fd': 'int'} }
>>  +{ 'command': 'remove-fd', 'data': {'fdset-id': 'int32', '*fd': 'int32'} }
>>
>>   ##
>>   # @FdsetFdInfo:
>>  @@ -2252,7 +2252,7 @@
>>   # Since: 1.2.0
>>   ##
>>   { 'struct': 'FdsetFdInfo',
>>  - 'data': {'fd': 'int', '*opaque': 'str'} }
>>  + 'data': {'fd': 'int32', '*opaque': 'str'} }
>>
>>   ##
>>   # @FdsetInfo:
>>  @@ -2266,7 +2266,7 @@
>>   # Since: 1.2.0
>>   ##
>>   { 'struct': 'FdsetInfo',
>>  - 'data': {'fdset-id': 'int', 'fds': ['FdsetFdInfo']} }
>>  + 'data': {'fdset-id': 'int32', 'fds': ['FdsetFdInfo']} }
>>
>>   ##
>>   # @query-fdsets:
>
> In the schema, you use QAPI type 'int32' instead of 'int'.
>
> Before the patch, the two are consistent (except for the bugs you fixed
> in v1 of this patch): QAPI 'int' is C int64_t.
>
> After the patch, the two are inconsistent: QAPI 'int32' is C int32_t,
> not int. They're usually the same, but it unclean.
>
> Two ways forward:
>
> 1. Revise this patch to use int32_t instead of int.
>
> 2. Revise v1 to address the few minor review comments I had. Smaller
>    patch, easier to review.
>
> Your choice. I'd choose 2.
>
> [...]

Sorry, I'm a little confused... I thought method 2 was not ok,
but if it’s not, then I prefer it too.

So, I will send v3 which is v1 with grammar fixes

Regards,
Yury


Re: [Qemu-devel] [PATCH v2] monitor: Fix fdset_id & fd types for corresponding QMP commands

Posted by Yury Kotov 9 weeks ago
Ping

14.05.2019, 18:20, "Yury Kotov" <yury-kotov@yandex-team.ru>:
> Now, fdset_id is int64, but in some places we work with it as int.
> It seems that there is no sense to use int64 for fdset_id, so it's
> better to fix inconsistency by changing fdset_id type to int and by
> fixing the reference of corresponding QMP commands: add-fd, remove-fd,
> query-fdsets.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
>  include/monitor/monitor.h | 6 +++---
>  monitor.c | 18 +++++++++---------
>  qapi/misc.json | 10 +++++-----
>  stubs/fdset.c | 4 ++--
>  util/osdep.c | 4 ++--
>  vl.c | 2 +-
>  6 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 86656297f1..06f9b6c217 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -39,11 +39,11 @@ void monitor_read_command(Monitor *mon, int show_prompt);
>  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>                            void *opaque);
>
> -AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> +AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int fdset_id,
>                                  bool has_opaque, const char *opaque,
>                                  Error **errp);
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> +int monitor_fdset_get_fd(int fdset_id, int flags);
> +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd);
>  void monitor_fdset_dup_fd_remove(int dup_fd);
>  int monitor_fdset_dup_fd_find(int dup_fd);
>
> diff --git a/monitor.c b/monitor.c
> index bb48997913..b71ce816bc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -160,7 +160,7 @@ struct MonFdsetFd {
>  /* file descriptor set containing fds passed via SCM_RIGHTS */
>  typedef struct MonFdset MonFdset;
>  struct MonFdset {
> - int64_t id;
> + int id;
>      QLIST_HEAD(, MonFdsetFd) fds;
>      QLIST_HEAD(, MonFdsetFd) dup_fds;
>      QLIST_ENTRY(MonFdset) next;
> @@ -2346,7 +2346,7 @@ static void monitor_fdsets_cleanup(void)
>      qemu_mutex_unlock(&mon_fdsets_lock);
>  }
>
> -AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
> +AddfdInfo *qmp_add_fd(bool has_fdset_id, int32_t fdset_id, bool has_opaque,
>                        const char *opaque, Error **errp)
>  {
>      int fd;
> @@ -2372,7 +2372,7 @@ error:
>      return NULL;
>  }
>
> -void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> +void qmp_remove_fd(int32_t fdset_id, bool has_fd, int fd, Error **errp)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd;
> @@ -2405,10 +2405,10 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
>  error:
>      qemu_mutex_unlock(&mon_fdsets_lock);
>      if (has_fd) {
> - snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64,
> + snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32 ", fd:%" PRId32,
>                   fdset_id, fd);
>      } else {
> - snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64, fdset_id);
> + snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId32, fdset_id);
>      }
>      error_setg(errp, QERR_FD_NOT_FOUND, fd_str);
>  }
> @@ -2454,7 +2454,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>      return fdset_list;
>  }
>
> -AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> +AddfdInfo *monitor_fdset_add_fd(int32_t fd, bool has_fdset_id, int32_t fdset_id,
>                                  bool has_opaque, const char *opaque,
>                                  Error **errp)
>  {
> @@ -2476,7 +2476,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>      }
>
>      if (mon_fdset == NULL) {
> - int64_t fdset_id_prev = -1;
> + int fdset_id_prev = -1;
>          MonFdset *mon_fdset_cur = QLIST_FIRST(&mon_fdsets);
>
>          if (has_fdset_id) {
> @@ -2538,7 +2538,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>      return fdinfo;
>  }
>
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> +int monitor_fdset_get_fd(int fdset_id, int flags)
>  {
>  #ifdef _WIN32
>      return -ENOENT;
> @@ -2576,7 +2576,7 @@ out:
>  #endif
>  }
>
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 8b3ca4fdd3..b345e1458b 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -2179,7 +2179,7 @@
>  #
>  # Since: 1.2.0
>  ##
> -{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int', 'fd': 'int'} }
> +{ 'struct': 'AddfdInfo', 'data': {'fdset-id': 'int32', 'fd': 'int32'} }
>
>  ##
>  # @add-fd:
> @@ -2209,7 +2209,7 @@
>  #
>  ##
>  { 'command': 'add-fd',
> - 'data': { '*fdset-id': 'int',
> + 'data': { '*fdset-id': 'int32',
>              '*opaque': 'str' },
>    'returns': 'AddfdInfo' }
>
> @@ -2238,7 +2238,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'remove-fd', 'data': {'fdset-id': 'int', '*fd': 'int'} }
> +{ 'command': 'remove-fd', 'data': {'fdset-id': 'int32', '*fd': 'int32'} }
>
>  ##
>  # @FdsetFdInfo:
> @@ -2252,7 +2252,7 @@
>  # Since: 1.2.0
>  ##
>  { 'struct': 'FdsetFdInfo',
> - 'data': {'fd': 'int', '*opaque': 'str'} }
> + 'data': {'fd': 'int32', '*opaque': 'str'} }
>
>  ##
>  # @FdsetInfo:
> @@ -2266,7 +2266,7 @@
>  # Since: 1.2.0
>  ##
>  { 'struct': 'FdsetInfo',
> - 'data': {'fdset-id': 'int', 'fds': ['FdsetFdInfo']} }
> + 'data': {'fdset-id': 'int32', 'fds': ['FdsetFdInfo']} }
>
>  ##
>  # @query-fdsets:
> diff --git a/stubs/fdset.c b/stubs/fdset.c
> index 4f3edf2ea4..1504624c19 100644
> --- a/stubs/fdset.c
> +++ b/stubs/fdset.c
> @@ -2,7 +2,7 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>
> -int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +int monitor_fdset_dup_fd_add(int fdset_id, int dup_fd)
>  {
>      return -1;
>  }
> @@ -12,7 +12,7 @@ int monitor_fdset_dup_fd_find(int dup_fd)
>      return -1;
>  }
>
> -int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> +int monitor_fdset_get_fd(int fdset_id, int flags)
>  {
>      return -ENOENT;
>  }
> diff --git a/util/osdep.c b/util/osdep.c
> index 3f04326040..9e2d3768e0 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -292,7 +292,7 @@ int qemu_open(const char *name, int flags, ...)
>
>      /* Attempt dup of fd from fd set */
>      if (strstart(name, "/dev/fdset/", &fdset_id_str)) {
> - int64_t fdset_id;
> + int fdset_id;
>          int fd, dupfd;
>
>          fdset_id = qemu_parse_fdset(fdset_id_str);
> @@ -352,7 +352,7 @@ int qemu_open(const char *name, int flags, ...)
>
>  int qemu_close(int fd)
>  {
> - int64_t fdset_id;
> + int fdset_id;
>
>      /* Close fd that was dup'd from an fdset */
>      fdset_id = monitor_fdset_dup_fd_find(fd);
> diff --git a/vl.c b/vl.c
> index b6709514c1..0f5622496c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1081,7 +1081,7 @@ bool defaults_enabled(void)
>  static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      int fd, dupfd, flags;
> - int64_t fdset_id;
> + int fdset_id;
>      const char *fd_opaque = NULL;
>      AddfdInfo *fdinfo;
>
> --
> 2.21.0