diff mbox

[v2,1/4] qapi: Convert getfd and closefd

Message ID 1339170179-2554-2-git-send-email-coreyb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Corey Bryant June 8, 2012, 3:42 p.m. UTC
v2:
 - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
 - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
 - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
 - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
 - Drop .user_print lines (lcapitulino@redhat.com)
 - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
 - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
 hmp-commands.hx  |    6 ++----
 hmp.c            |   18 ++++++++++++++++++
 hmp.h            |    2 ++
 monitor.c        |   34 ++++++++++++++++------------------
 qapi-schema.json |   29 +++++++++++++++++++++++++++++
 qmp-commands.hx  |    6 ++----
 6 files changed, 69 insertions(+), 26 deletions(-)

Comments

Luiz Capitulino June 13, 2012, 7:41 p.m. UTC | #1
On Fri,  8 Jun 2012 11:42:56 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> v2:
>  - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
>  - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
>  - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
>  - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
>  - Drop .user_print lines (lcapitulino@redhat.com)
>  - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
>  - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)
> 
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |    6 ++----
>  hmp.c            |   18 ++++++++++++++++++
>  hmp.h            |    2 ++
>  monitor.c        |   34 ++++++++++++++++------------------
>  qapi-schema.json |   29 +++++++++++++++++++++++++++++
>  qmp-commands.hx  |    6 ++----
>  6 files changed, 69 insertions(+), 26 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f5d9d91..eea8b32 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1236,8 +1236,7 @@ ETEXI
>          .args_type  = "fdname:s",
>          .params     = "getfd name",
>          .help       = "receive a file descriptor via SCM rights and assign it a name",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_getfd,
> +        .mhandler.cmd = hmp_getfd,
>      },
>  
>  STEXI
> @@ -1253,8 +1252,7 @@ ETEXI
>          .args_type  = "fdname:s",
>          .params     = "closefd name",
>          .help       = "close a file descriptor previously passed via SCM rights",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_closefd,
> +        .mhandler.cmd = hmp_closefd,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 2ce8cb9..6241856 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>      qmp_netdev_del(id, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_getfd(Monitor *mon, const QDict *qdict)
> +{
> +    const char *fdname = qdict_get_str(qdict, "fdname");
> +    Error *errp = NULL;
> +
> +    qmp_getfd(fdname, &errp);
> +    hmp_handle_error(mon, &errp);
> +}
> +
> +void hmp_closefd(Monitor *mon, const QDict *qdict)
> +{
> +    const char *fdname = qdict_get_str(qdict, "fdname");
> +    Error *errp = NULL;
> +
> +    qmp_closefd(fdname, &errp);
> +    hmp_handle_error(mon, &errp);
> +}
> diff --git a/hmp.h b/hmp.h
> index 79d138d..8d2b0d7 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_getfd(Monitor *mon, const QDict *qdict);
> +void hmp_closefd(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index a3bc2c7..4c53cb6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2182,48 +2182,46 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>  }
>  #endif
>  
> -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_getfd(const char *fdname, Error **errp)
>  {
> -    const char *fdname = qdict_get_str(qdict, "fdname");
>      mon_fd_t *monfd;
>      int fd;
>  
> -    fd = qemu_chr_fe_get_msgfd(mon->chr);
> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
>      if (fd == -1) {
> -        qerror_report(QERR_FD_NOT_SUPPLIED);
> -        return -1;
> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> +        return;
>      }
>  
>      if (qemu_isdigit(fdname[0])) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
> -                      "a name not starting with a digit");
> -        return -1;
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> +                  "a name not starting with a digit");
> +        return;
>      }
>  
> -    QLIST_FOREACH(monfd, &mon->fds, next) {
> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
>          }
>  
>          close(monfd->fd);
>          monfd->fd = fd;
> -        return 0;
> +        return;
>      }
>  
>      monfd = g_malloc0(sizeof(mon_fd_t));
>      monfd->name = g_strdup(fdname);
>      monfd->fd = fd;
>  
> -    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> -    return 0;
> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> +    return;

You have a few of this kind of gratuitous return, please drop 'em.

>  }
>  
> -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_closefd(const char *fdname, Error **errp)
>  {
> -    const char *fdname = qdict_get_str(qdict, "fdname");
>      mon_fd_t *monfd;
>  
> -    QLIST_FOREACH(monfd, &mon->fds, next) {
> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
>          }
> @@ -2232,11 +2230,11 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          close(monfd->fd);
>          g_free(monfd->name);
>          g_free(monfd);
> -        return 0;
> +        return;
>      }
>  
> -    qerror_report(QERR_FD_NOT_FOUND, fdname);
> -    return -1;
> +    error_set(errp, QERR_FD_NOT_FOUND, fdname);
> +    return;
>  }
>  
>  static void do_loadvm(Monitor *mon, const QDict *qdict)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3b6e346..6be1d90 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1862,3 +1862,32 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> +
> +##
> +# @getfd:
> +#
> +# Receive a file descriptor via SCM rights and assign it a name
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: Nothing on success
> +#          If file descriptor was not received, FdNotSupplied
> +#          If @fdname is not valid, InvalidParameterType

Please, add a note explaining that getfd automatically closes fds if
fdname matches an existing name.

Otherwise this conversion looks very good to me.

> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> +
> +##
> +# @closefd:
> +#
> +# Close a file descriptor previously passed via SCM rights
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: Nothing on success
> +#          If @fdname is not found, FdNotFound
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'closefd', 'data': {'fdname': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e1a38e..f8c0f68 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -873,8 +873,7 @@ EQMP
>          .args_type  = "fdname:s",
>          .params     = "getfd name",
>          .help       = "receive a file descriptor via SCM rights and assign it a name",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_getfd,
> +        .mhandler.cmd_new = qmp_marshal_input_getfd,
>      },
>  
>  SQMP
> @@ -899,8 +898,7 @@ EQMP
>          .args_type  = "fdname:s",
>          .params     = "closefd name",
>          .help       = "close a file descriptor previously passed via SCM rights",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_closefd,
> +        .mhandler.cmd_new = qmp_marshal_input_closefd,
>      },
>  
>  SQMP
Luiz Capitulino June 13, 2012, 7:42 p.m. UTC | #2
On Fri,  8 Jun 2012 11:42:56 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> v2:
>  - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
>  - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
>  - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
>  - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
>  - Drop .user_print lines (lcapitulino@redhat.com)
>  - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
>  - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)

Btw, having the changelog like this is not nice because it becomes part
of the history. It's better to move it after the '---' line, so that
git ignores it.

> 
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx  |    6 ++----
>  hmp.c            |   18 ++++++++++++++++++
>  hmp.h            |    2 ++
>  monitor.c        |   34 ++++++++++++++++------------------
>  qapi-schema.json |   29 +++++++++++++++++++++++++++++
>  qmp-commands.hx  |    6 ++----
>  6 files changed, 69 insertions(+), 26 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f5d9d91..eea8b32 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1236,8 +1236,7 @@ ETEXI
>          .args_type  = "fdname:s",
>          .params     = "getfd name",
>          .help       = "receive a file descriptor via SCM rights and assign it a name",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_getfd,
> +        .mhandler.cmd = hmp_getfd,
>      },
>  
>  STEXI
> @@ -1253,8 +1252,7 @@ ETEXI
>          .args_type  = "fdname:s",
>          .params     = "closefd name",
>          .help       = "close a file descriptor previously passed via SCM rights",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_closefd,
> +        .mhandler.cmd = hmp_closefd,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 2ce8cb9..6241856 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>      qmp_netdev_del(id, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_getfd(Monitor *mon, const QDict *qdict)
> +{
> +    const char *fdname = qdict_get_str(qdict, "fdname");
> +    Error *errp = NULL;
> +
> +    qmp_getfd(fdname, &errp);
> +    hmp_handle_error(mon, &errp);
> +}
> +
> +void hmp_closefd(Monitor *mon, const QDict *qdict)
> +{
> +    const char *fdname = qdict_get_str(qdict, "fdname");
> +    Error *errp = NULL;
> +
> +    qmp_closefd(fdname, &errp);
> +    hmp_handle_error(mon, &errp);
> +}
> diff --git a/hmp.h b/hmp.h
> index 79d138d..8d2b0d7 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_getfd(Monitor *mon, const QDict *qdict);
> +void hmp_closefd(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index a3bc2c7..4c53cb6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2182,48 +2182,46 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>  }
>  #endif
>  
> -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_getfd(const char *fdname, Error **errp)
>  {
> -    const char *fdname = qdict_get_str(qdict, "fdname");
>      mon_fd_t *monfd;
>      int fd;
>  
> -    fd = qemu_chr_fe_get_msgfd(mon->chr);
> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
>      if (fd == -1) {
> -        qerror_report(QERR_FD_NOT_SUPPLIED);
> -        return -1;
> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> +        return;
>      }
>  
>      if (qemu_isdigit(fdname[0])) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
> -                      "a name not starting with a digit");
> -        return -1;
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> +                  "a name not starting with a digit");
> +        return;
>      }
>  
> -    QLIST_FOREACH(monfd, &mon->fds, next) {
> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
>          }
>  
>          close(monfd->fd);
>          monfd->fd = fd;
> -        return 0;
> +        return;
>      }
>  
>      monfd = g_malloc0(sizeof(mon_fd_t));
>      monfd->name = g_strdup(fdname);
>      monfd->fd = fd;
>  
> -    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> -    return 0;
> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> +    return;
>  }
>  
> -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_closefd(const char *fdname, Error **errp)
>  {
> -    const char *fdname = qdict_get_str(qdict, "fdname");
>      mon_fd_t *monfd;
>  
> -    QLIST_FOREACH(monfd, &mon->fds, next) {
> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
>          }
> @@ -2232,11 +2230,11 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          close(monfd->fd);
>          g_free(monfd->name);
>          g_free(monfd);
> -        return 0;
> +        return;
>      }
>  
> -    qerror_report(QERR_FD_NOT_FOUND, fdname);
> -    return -1;
> +    error_set(errp, QERR_FD_NOT_FOUND, fdname);
> +    return;
>  }
>  
>  static void do_loadvm(Monitor *mon, const QDict *qdict)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3b6e346..6be1d90 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1862,3 +1862,32 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> +
> +##
> +# @getfd:
> +#
> +# Receive a file descriptor via SCM rights and assign it a name
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: Nothing on success
> +#          If file descriptor was not received, FdNotSupplied
> +#          If @fdname is not valid, InvalidParameterType
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> +
> +##
> +# @closefd:
> +#
> +# Close a file descriptor previously passed via SCM rights
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: Nothing on success
> +#          If @fdname is not found, FdNotFound
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'closefd', 'data': {'fdname': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e1a38e..f8c0f68 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -873,8 +873,7 @@ EQMP
>          .args_type  = "fdname:s",
>          .params     = "getfd name",
>          .help       = "receive a file descriptor via SCM rights and assign it a name",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_getfd,
> +        .mhandler.cmd_new = qmp_marshal_input_getfd,
>      },
>  
>  SQMP
> @@ -899,8 +898,7 @@ EQMP
>          .args_type  = "fdname:s",
>          .params     = "closefd name",
>          .help       = "close a file descriptor previously passed via SCM rights",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_closefd,
> +        .mhandler.cmd_new = qmp_marshal_input_closefd,
>      },
>  
>  SQMP
Corey Bryant June 13, 2012, 8:10 p.m. UTC | #3
On 06/13/2012 03:41 PM, Luiz Capitulino wrote:
> On Fri,  8 Jun 2012 11:42:56 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>> v2:
>>   - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
>>   - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
>>   - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
>>   - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
>>   - Drop .user_print lines (lcapitulino@redhat.com)
>>   - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
>>   - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)
>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx  |    6 ++----
>>   hmp.c            |   18 ++++++++++++++++++
>>   hmp.h            |    2 ++
>>   monitor.c        |   34 ++++++++++++++++------------------
>>   qapi-schema.json |   29 +++++++++++++++++++++++++++++
>>   qmp-commands.hx  |    6 ++----
>>   6 files changed, 69 insertions(+), 26 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index f5d9d91..eea8b32 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1236,8 +1236,7 @@ ETEXI
>>           .args_type  = "fdname:s",
>>           .params     = "getfd name",
>>           .help       = "receive a file descriptor via SCM rights and assign it a name",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_getfd,
>> +        .mhandler.cmd = hmp_getfd,
>>       },
>>
>>   STEXI
>> @@ -1253,8 +1252,7 @@ ETEXI
>>           .args_type  = "fdname:s",
>>           .params     = "closefd name",
>>           .help       = "close a file descriptor previously passed via SCM rights",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_closefd,
>> +        .mhandler.cmd = hmp_closefd,
>>       },
>>
>>   STEXI
>> diff --git a/hmp.c b/hmp.c
>> index 2ce8cb9..6241856 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>>       qmp_netdev_del(id, &err);
>>       hmp_handle_error(mon, &err);
>>   }
>> +
>> +void hmp_getfd(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *fdname = qdict_get_str(qdict, "fdname");
>> +    Error *errp = NULL;
>> +
>> +    qmp_getfd(fdname, &errp);
>> +    hmp_handle_error(mon, &errp);
>> +}
>> +
>> +void hmp_closefd(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *fdname = qdict_get_str(qdict, "fdname");
>> +    Error *errp = NULL;
>> +
>> +    qmp_closefd(fdname, &errp);
>> +    hmp_handle_error(mon, &errp);
>> +}
>> diff --git a/hmp.h b/hmp.h
>> index 79d138d..8d2b0d7 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>>   void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>>   void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>>   void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>> +void hmp_getfd(Monitor *mon, const QDict *qdict);
>> +void hmp_closefd(Monitor *mon, const QDict *qdict);
>>
>>   #endif
>> diff --git a/monitor.c b/monitor.c
>> index a3bc2c7..4c53cb6 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2182,48 +2182,46 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>>   }
>>   #endif
>>
>> -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +void qmp_getfd(const char *fdname, Error **errp)
>>   {
>> -    const char *fdname = qdict_get_str(qdict, "fdname");
>>       mon_fd_t *monfd;
>>       int fd;
>>
>> -    fd = qemu_chr_fe_get_msgfd(mon->chr);
>> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
>>       if (fd == -1) {
>> -        qerror_report(QERR_FD_NOT_SUPPLIED);
>> -        return -1;
>> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
>> +        return;
>>       }
>>
>>       if (qemu_isdigit(fdname[0])) {
>> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
>> -                      "a name not starting with a digit");
>> -        return -1;
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>> +                  "a name not starting with a digit");
>> +        return;
>>       }
>>
>> -    QLIST_FOREACH(monfd, &mon->fds, next) {
>> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>>           if (strcmp(monfd->name, fdname) != 0) {
>>               continue;
>>           }
>>
>>           close(monfd->fd);
>>           monfd->fd = fd;
>> -        return 0;
>> +        return;
>>       }
>>
>>       monfd = g_malloc0(sizeof(mon_fd_t));
>>       monfd->name = g_strdup(fdname);
>>       monfd->fd = fd;
>>
>> -    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
>> -    return 0;
>> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>> +    return;
>
> You have a few of this kind of gratuitous return, please drop 'em.
>

Thanks for reviewing.

I'll get a v3 patch series going and remove the unnecessary returns in it.

>>   }
>>
>> -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +void qmp_closefd(const char *fdname, Error **errp)
>>   {
>> -    const char *fdname = qdict_get_str(qdict, "fdname");
>>       mon_fd_t *monfd;
>>
>> -    QLIST_FOREACH(monfd, &mon->fds, next) {
>> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>>           if (strcmp(monfd->name, fdname) != 0) {
>>               continue;
>>           }
>> @@ -2232,11 +2230,11 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>           close(monfd->fd);
>>           g_free(monfd->name);
>>           g_free(monfd);
>> -        return 0;
>> +        return;
>>       }
>>
>> -    qerror_report(QERR_FD_NOT_FOUND, fdname);
>> -    return -1;
>> +    error_set(errp, QERR_FD_NOT_FOUND, fdname);
>> +    return;
>>   }
>>
>>   static void do_loadvm(Monitor *mon, const QDict *qdict)
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 3b6e346..6be1d90 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1862,3 +1862,32 @@
>>   # Since: 0.14.0
>>   ##
>>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
>> +
>> +##
>> +# @getfd:
>> +#
>> +# Receive a file descriptor via SCM rights and assign it a name
>> +#
>> +# @fdname: file descriptor name
>> +#
>> +# Returns: Nothing on success
>> +#          If file descriptor was not received, FdNotSupplied
>> +#          If @fdname is not valid, InvalidParameterType
>
> Please, add a note explaining that getfd automatically closes fds if
> fdname matches an existing name.

Ok

>
> Otherwise this conversion looks very good to me.

Good to hear!

>
>> +#
>> +# Since: 0.14.0
>> +##
>> +{ 'command': 'getfd', 'data': {'fdname': 'str'} }
>> +
>> +##
>> +# @closefd:
>> +#
>> +# Close a file descriptor previously passed via SCM rights
>> +#
>> +# @fdname: file descriptor name
>> +#
>> +# Returns: Nothing on success
>> +#          If @fdname is not found, FdNotFound
>> +#
>> +# Since: 0.14.0
>> +##
>> +{ 'command': 'closefd', 'data': {'fdname': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 2e1a38e..f8c0f68 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -873,8 +873,7 @@ EQMP
>>           .args_type  = "fdname:s",
>>           .params     = "getfd name",
>>           .help       = "receive a file descriptor via SCM rights and assign it a name",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_getfd,
>> +        .mhandler.cmd_new = qmp_marshal_input_getfd,
>>       },
>>
>>   SQMP
>> @@ -899,8 +898,7 @@ EQMP
>>           .args_type  = "fdname:s",
>>           .params     = "closefd name",
>>           .help       = "close a file descriptor previously passed via SCM rights",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_closefd,
>> +        .mhandler.cmd_new = qmp_marshal_input_closefd,
>>       },
>>
>>   SQMP
>
Corey Bryant June 13, 2012, 8:17 p.m. UTC | #4
On 06/13/2012 03:42 PM, Luiz Capitulino wrote:
> On Fri,  8 Jun 2012 11:42:56 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>> v2:
>>   - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
>>   - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
>>   - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
>>   - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
>>   - Drop .user_print lines (lcapitulino@redhat.com)
>>   - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
>>   - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)
>
> Btw, having the changelog like this is not nice because it becomes part
> of the history. It's better to move it after the '---' line, so that
> git ignores it.
>

I see your point and I can do this in v3.  But can I add text after the 
'---' line in the commit message via 'git commit' or do I have to 
manually edit the patches?

>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx  |    6 ++----
>>   hmp.c            |   18 ++++++++++++++++++
>>   hmp.h            |    2 ++
>>   monitor.c        |   34 ++++++++++++++++------------------
>>   qapi-schema.json |   29 +++++++++++++++++++++++++++++
>>   qmp-commands.hx  |    6 ++----
>>   6 files changed, 69 insertions(+), 26 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index f5d9d91..eea8b32 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1236,8 +1236,7 @@ ETEXI
>>           .args_type  = "fdname:s",
>>           .params     = "getfd name",
>>           .help       = "receive a file descriptor via SCM rights and assign it a name",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_getfd,
>> +        .mhandler.cmd = hmp_getfd,
>>       },
>>
>>   STEXI
>> @@ -1253,8 +1252,7 @@ ETEXI
>>           .args_type  = "fdname:s",
>>           .params     = "closefd name",
>>           .help       = "close a file descriptor previously passed via SCM rights",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_closefd,
>> +        .mhandler.cmd = hmp_closefd,
>>       },
>>
>>   STEXI
>> diff --git a/hmp.c b/hmp.c
>> index 2ce8cb9..6241856 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>>       qmp_netdev_del(id, &err);
>>       hmp_handle_error(mon, &err);
>>   }
>> +
>> +void hmp_getfd(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *fdname = qdict_get_str(qdict, "fdname");
>> +    Error *errp = NULL;
>> +
>> +    qmp_getfd(fdname, &errp);
>> +    hmp_handle_error(mon, &errp);
>> +}
>> +
>> +void hmp_closefd(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *fdname = qdict_get_str(qdict, "fdname");
>> +    Error *errp = NULL;
>> +
>> +    qmp_closefd(fdname, &errp);
>> +    hmp_handle_error(mon, &errp);
>> +}
>> diff --git a/hmp.h b/hmp.h
>> index 79d138d..8d2b0d7 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>>   void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>>   void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>>   void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>> +void hmp_getfd(Monitor *mon, const QDict *qdict);
>> +void hmp_closefd(Monitor *mon, const QDict *qdict);
>>
>>   #endif
>> diff --git a/monitor.c b/monitor.c
>> index a3bc2c7..4c53cb6 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2182,48 +2182,46 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>>   }
>>   #endif
>>
>> -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +void qmp_getfd(const char *fdname, Error **errp)
>>   {
>> -    const char *fdname = qdict_get_str(qdict, "fdname");
>>       mon_fd_t *monfd;
>>       int fd;
>>
>> -    fd = qemu_chr_fe_get_msgfd(mon->chr);
>> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
>>       if (fd == -1) {
>> -        qerror_report(QERR_FD_NOT_SUPPLIED);
>> -        return -1;
>> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
>> +        return;
>>       }
>>
>>       if (qemu_isdigit(fdname[0])) {
>> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
>> -                      "a name not starting with a digit");
>> -        return -1;
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>> +                  "a name not starting with a digit");
>> +        return;
>>       }
>>
>> -    QLIST_FOREACH(monfd, &mon->fds, next) {
>> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>>           if (strcmp(monfd->name, fdname) != 0) {
>>               continue;
>>           }
>>
>>           close(monfd->fd);
>>           monfd->fd = fd;
>> -        return 0;
>> +        return;
>>       }
>>
>>       monfd = g_malloc0(sizeof(mon_fd_t));
>>       monfd->name = g_strdup(fdname);
>>       monfd->fd = fd;
>>
>> -    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
>> -    return 0;
>> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>> +    return;
>>   }
>>
>> -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> +void qmp_closefd(const char *fdname, Error **errp)
>>   {
>> -    const char *fdname = qdict_get_str(qdict, "fdname");
>>       mon_fd_t *monfd;
>>
>> -    QLIST_FOREACH(monfd, &mon->fds, next) {
>> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>>           if (strcmp(monfd->name, fdname) != 0) {
>>               continue;
>>           }
>> @@ -2232,11 +2230,11 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>           close(monfd->fd);
>>           g_free(monfd->name);
>>           g_free(monfd);
>> -        return 0;
>> +        return;
>>       }
>>
>> -    qerror_report(QERR_FD_NOT_FOUND, fdname);
>> -    return -1;
>> +    error_set(errp, QERR_FD_NOT_FOUND, fdname);
>> +    return;
>>   }
>>
>>   static void do_loadvm(Monitor *mon, const QDict *qdict)
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 3b6e346..6be1d90 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1862,3 +1862,32 @@
>>   # Since: 0.14.0
>>   ##
>>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
>> +
>> +##
>> +# @getfd:
>> +#
>> +# Receive a file descriptor via SCM rights and assign it a name
>> +#
>> +# @fdname: file descriptor name
>> +#
>> +# Returns: Nothing on success
>> +#          If file descriptor was not received, FdNotSupplied
>> +#          If @fdname is not valid, InvalidParameterType
>> +#
>> +# Since: 0.14.0
>> +##
>> +{ 'command': 'getfd', 'data': {'fdname': 'str'} }
>> +
>> +##
>> +# @closefd:
>> +#
>> +# Close a file descriptor previously passed via SCM rights
>> +#
>> +# @fdname: file descriptor name
>> +#
>> +# Returns: Nothing on success
>> +#          If @fdname is not found, FdNotFound
>> +#
>> +# Since: 0.14.0
>> +##
>> +{ 'command': 'closefd', 'data': {'fdname': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 2e1a38e..f8c0f68 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -873,8 +873,7 @@ EQMP
>>           .args_type  = "fdname:s",
>>           .params     = "getfd name",
>>           .help       = "receive a file descriptor via SCM rights and assign it a name",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_getfd,
>> +        .mhandler.cmd_new = qmp_marshal_input_getfd,
>>       },
>>
>>   SQMP
>> @@ -899,8 +898,7 @@ EQMP
>>           .args_type  = "fdname:s",
>>           .params     = "closefd name",
>>           .help       = "close a file descriptor previously passed via SCM rights",
>> -        .user_print = monitor_user_noop,
>> -        .mhandler.cmd_new = do_closefd,
>> +        .mhandler.cmd_new = qmp_marshal_input_closefd,
>>       },
>>
>>   SQMP
>
>
Luiz Capitulino June 13, 2012, 8:41 p.m. UTC | #5
On Wed, 13 Jun 2012 16:17:30 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> 
> 
> On 06/13/2012 03:42 PM, Luiz Capitulino wrote:
> > On Fri,  8 Jun 2012 11:42:56 -0400
> > Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
> >
> >> v2:
> >>   - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
> >>   - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
> >>   - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
> >>   - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
> >>   - Drop .user_print lines (lcapitulino@redhat.com)
> >>   - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
> >>   - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)
> >
> > Btw, having the changelog like this is not nice because it becomes part
> > of the history. It's better to move it after the '---' line, so that
> > git ignores it.
> >
> 
> I see your point and I can do this in v3.  But can I add text after the 
> '---' line in the commit message via 'git commit' or do I have to 
> manually edit the patches?

I don't know of an automated way of doing this, I'd export the patches with
git format-patch and edit them manually.

But I usually put changelog in the intro patch.

> 
> >>
> >> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> >> ---
> >>   hmp-commands.hx  |    6 ++----
> >>   hmp.c            |   18 ++++++++++++++++++
> >>   hmp.h            |    2 ++
> >>   monitor.c        |   34 ++++++++++++++++------------------
> >>   qapi-schema.json |   29 +++++++++++++++++++++++++++++
> >>   qmp-commands.hx  |    6 ++----
> >>   6 files changed, 69 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/hmp-commands.hx b/hmp-commands.hx
> >> index f5d9d91..eea8b32 100644
> >> --- a/hmp-commands.hx
> >> +++ b/hmp-commands.hx
> >> @@ -1236,8 +1236,7 @@ ETEXI
> >>           .args_type  = "fdname:s",
> >>           .params     = "getfd name",
> >>           .help       = "receive a file descriptor via SCM rights and assign it a name",
> >> -        .user_print = monitor_user_noop,
> >> -        .mhandler.cmd_new = do_getfd,
> >> +        .mhandler.cmd = hmp_getfd,
> >>       },
> >>
> >>   STEXI
> >> @@ -1253,8 +1252,7 @@ ETEXI
> >>           .args_type  = "fdname:s",
> >>           .params     = "closefd name",
> >>           .help       = "close a file descriptor previously passed via SCM rights",
> >> -        .user_print = monitor_user_noop,
> >> -        .mhandler.cmd_new = do_closefd,
> >> +        .mhandler.cmd = hmp_closefd,
> >>       },
> >>
> >>   STEXI
> >> diff --git a/hmp.c b/hmp.c
> >> index 2ce8cb9..6241856 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> >>       qmp_netdev_del(id, &err);
> >>       hmp_handle_error(mon, &err);
> >>   }
> >> +
> >> +void hmp_getfd(Monitor *mon, const QDict *qdict)
> >> +{
> >> +    const char *fdname = qdict_get_str(qdict, "fdname");
> >> +    Error *errp = NULL;
> >> +
> >> +    qmp_getfd(fdname, &errp);
> >> +    hmp_handle_error(mon, &errp);
> >> +}
> >> +
> >> +void hmp_closefd(Monitor *mon, const QDict *qdict)
> >> +{
> >> +    const char *fdname = qdict_get_str(qdict, "fdname");
> >> +    Error *errp = NULL;
> >> +
> >> +    qmp_closefd(fdname, &errp);
> >> +    hmp_handle_error(mon, &errp);
> >> +}
> >> diff --git a/hmp.h b/hmp.h
> >> index 79d138d..8d2b0d7 100644
> >> --- a/hmp.h
> >> +++ b/hmp.h
> >> @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
> >>   void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> >>   void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> >>   void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> >> +void hmp_getfd(Monitor *mon, const QDict *qdict);
> >> +void hmp_closefd(Monitor *mon, const QDict *qdict);
> >>
> >>   #endif
> >> diff --git a/monitor.c b/monitor.c
> >> index a3bc2c7..4c53cb6 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -2182,48 +2182,46 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
> >>   }
> >>   #endif
> >>
> >> -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >> +void qmp_getfd(const char *fdname, Error **errp)
> >>   {
> >> -    const char *fdname = qdict_get_str(qdict, "fdname");
> >>       mon_fd_t *monfd;
> >>       int fd;
> >>
> >> -    fd = qemu_chr_fe_get_msgfd(mon->chr);
> >> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
> >>       if (fd == -1) {
> >> -        qerror_report(QERR_FD_NOT_SUPPLIED);
> >> -        return -1;
> >> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> >> +        return;
> >>       }
> >>
> >>       if (qemu_isdigit(fdname[0])) {
> >> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
> >> -                      "a name not starting with a digit");
> >> -        return -1;
> >> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> >> +                  "a name not starting with a digit");
> >> +        return;
> >>       }
> >>
> >> -    QLIST_FOREACH(monfd, &mon->fds, next) {
> >> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >>           if (strcmp(monfd->name, fdname) != 0) {
> >>               continue;
> >>           }
> >>
> >>           close(monfd->fd);
> >>           monfd->fd = fd;
> >> -        return 0;
> >> +        return;
> >>       }
> >>
> >>       monfd = g_malloc0(sizeof(mon_fd_t));
> >>       monfd->name = g_strdup(fdname);
> >>       monfd->fd = fd;
> >>
> >> -    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
> >> -    return 0;
> >> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> >> +    return;
> >>   }
> >>
> >> -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >> +void qmp_closefd(const char *fdname, Error **errp)
> >>   {
> >> -    const char *fdname = qdict_get_str(qdict, "fdname");
> >>       mon_fd_t *monfd;
> >>
> >> -    QLIST_FOREACH(monfd, &mon->fds, next) {
> >> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >>           if (strcmp(monfd->name, fdname) != 0) {
> >>               continue;
> >>           }
> >> @@ -2232,11 +2230,11 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >>           close(monfd->fd);
> >>           g_free(monfd->name);
> >>           g_free(monfd);
> >> -        return 0;
> >> +        return;
> >>       }
> >>
> >> -    qerror_report(QERR_FD_NOT_FOUND, fdname);
> >> -    return -1;
> >> +    error_set(errp, QERR_FD_NOT_FOUND, fdname);
> >> +    return;
> >>   }
> >>
> >>   static void do_loadvm(Monitor *mon, const QDict *qdict)
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 3b6e346..6be1d90 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -1862,3 +1862,32 @@
> >>   # Since: 0.14.0
> >>   ##
> >>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >> +
> >> +##
> >> +# @getfd:
> >> +#
> >> +# Receive a file descriptor via SCM rights and assign it a name
> >> +#
> >> +# @fdname: file descriptor name
> >> +#
> >> +# Returns: Nothing on success
> >> +#          If file descriptor was not received, FdNotSupplied
> >> +#          If @fdname is not valid, InvalidParameterType
> >> +#
> >> +# Since: 0.14.0
> >> +##
> >> +{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> >> +
> >> +##
> >> +# @closefd:
> >> +#
> >> +# Close a file descriptor previously passed via SCM rights
> >> +#
> >> +# @fdname: file descriptor name
> >> +#
> >> +# Returns: Nothing on success
> >> +#          If @fdname is not found, FdNotFound
> >> +#
> >> +# Since: 0.14.0
> >> +##
> >> +{ 'command': 'closefd', 'data': {'fdname': 'str'} }
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index 2e1a38e..f8c0f68 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -873,8 +873,7 @@ EQMP
> >>           .args_type  = "fdname:s",
> >>           .params     = "getfd name",
> >>           .help       = "receive a file descriptor via SCM rights and assign it a name",
> >> -        .user_print = monitor_user_noop,
> >> -        .mhandler.cmd_new = do_getfd,
> >> +        .mhandler.cmd_new = qmp_marshal_input_getfd,
> >>       },
> >>
> >>   SQMP
> >> @@ -899,8 +898,7 @@ EQMP
> >>           .args_type  = "fdname:s",
> >>           .params     = "closefd name",
> >>           .help       = "close a file descriptor previously passed via SCM rights",
> >> -        .user_print = monitor_user_noop,
> >> -        .mhandler.cmd_new = do_closefd,
> >> +        .mhandler.cmd_new = qmp_marshal_input_closefd,
> >>       },
> >>
> >>   SQMP
> >
> >
>
Eric Blake June 13, 2012, 8:41 p.m. UTC | #6
On 06/13/2012 02:17 PM, Corey Bryant wrote:
> 
> 
> On 06/13/2012 03:42 PM, Luiz Capitulino wrote:
>> On Fri,  8 Jun 2012 11:42:56 -0400
>> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>
>>> v2:
>>>   - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
>>>   - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
>>>   - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
>>>   - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
>>>   - Drop .user_print lines (lcapitulino@redhat.com)
>>>   - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
>>>   - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)
>>
>> Btw, having the changelog like this is not nice because it becomes part
>> of the history. It's better to move it after the '---' line, so that
>> git ignores it.
>>
> 
> I see your point and I can do this in v3.  But can I add text after the
> '---' line in the commit message via 'git commit' or do I have to
> manually edit the patches?

I also like tracking my notes to self/reviewers in the commit message as
I do a rebase.  'git send-email' automatically adds --- at the end of
your commit message, so I personally end up using 'git send-email
--annotate' and manually move the --- line to occur before my separation
point.  I think you can also stick --- in the middle of your commit
message at which point 'git am' will truncate from the first instance
when applying your email, without you having to edit things when
mailing, although I haven't tried it myself (at any rate, I have seen
patches from others with double --- lines, and assume that the doubled
line is a result of the literal --- line in their commit message).

Supposedly, it is also possible to use 'git notes' coupled with
notes.rewrite* options in your .gitconfig to track your notes over a
rebase, as well as an undocumented option to 'git send-email' to have
your notes automatically included after a lone --- line, but that are of
git is woefully under-documented and probably has issues that need
fixing before turning it into a daily workflow.
Corey Bryant June 13, 2012, 9:43 p.m. UTC | #7
On 06/13/2012 04:41 PM, Eric Blake wrote:
> On 06/13/2012 02:17 PM, Corey Bryant wrote:
>>
>>
>> On 06/13/2012 03:42 PM, Luiz Capitulino wrote:
>>> On Fri,  8 Jun 2012 11:42:56 -0400
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>>
>>>> v2:
>>>>    - Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
>>>>    - Remove changes that returned fd from getfd (lcapitulino@redhat.com)
>>>>    - Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
>>>>    - Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
>>>>    - Drop .user_print lines (lcapitulino@redhat.com)
>>>>    - Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
>>>>    - Change QMP command existance back to 0.14 (lcapitulino@redhat.com)
>>>
>>> Btw, having the changelog like this is not nice because it becomes part
>>> of the history. It's better to move it after the '---' line, so that
>>> git ignores it.
>>>
>>
>> I see your point and I can do this in v3.  But can I add text after the
>> '---' line in the commit message via 'git commit' or do I have to
>> manually edit the patches?
>
> I also like tracking my notes to self/reviewers in the commit message as
> I do a rebase.  'git send-email' automatically adds --- at the end of
> your commit message, so I personally end up using 'git send-email
> --annotate' and manually move the --- line to occur before my separation
> point.  I think you can also stick --- in the middle of your commit
> message at which point 'git am' will truncate from the first instance
> when applying your email, without you having to edit things when
> mailing, although I haven't tried it myself (at any rate, I have seen
> patches from others with double --- lines, and assume that the doubled
> line is a result of the literal --- line in their commit message).
>
> Supposedly, it is also possible to use 'git notes' coupled with
> notes.rewrite* options in your .gitconfig to track your notes over a
> rebase, as well as an undocumented option to 'git send-email' to have
> your notes automatically included after a lone --- line, but that are of
> git is woefully under-documented and probably has issues that need
> fixing before turning it into a daily workflow.
>

Thanks for the tips!
diff mbox

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index f5d9d91..eea8b32 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1236,8 +1236,7 @@  ETEXI
         .args_type  = "fdname:s",
         .params     = "getfd name",
         .help       = "receive a file descriptor via SCM rights and assign it a name",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_getfd,
+        .mhandler.cmd = hmp_getfd,
     },
 
 STEXI
@@ -1253,8 +1252,7 @@  ETEXI
         .args_type  = "fdname:s",
         .params     = "closefd name",
         .help       = "close a file descriptor previously passed via SCM rights",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_closefd,
+        .mhandler.cmd = hmp_closefd,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 2ce8cb9..6241856 100644
--- a/hmp.c
+++ b/hmp.c
@@ -999,3 +999,21 @@  void hmp_netdev_del(Monitor *mon, const QDict *qdict)
     qmp_netdev_del(id, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_getfd(Monitor *mon, const QDict *qdict)
+{
+    const char *fdname = qdict_get_str(qdict, "fdname");
+    Error *errp = NULL;
+
+    qmp_getfd(fdname, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
+void hmp_closefd(Monitor *mon, const QDict *qdict)
+{
+    const char *fdname = qdict_get_str(qdict, "fdname");
+    Error *errp = NULL;
+
+    qmp_closefd(fdname, &errp);
+    hmp_handle_error(mon, &errp);
+}
diff --git a/hmp.h b/hmp.h
index 79d138d..8d2b0d7 100644
--- a/hmp.h
+++ b/hmp.h
@@ -64,5 +64,7 @@  void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
+void hmp_getfd(Monitor *mon, const QDict *qdict);
+void hmp_closefd(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index a3bc2c7..4c53cb6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2182,48 +2182,46 @@  static void do_inject_mce(Monitor *mon, const QDict *qdict)
 }
 #endif
 
-static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_getfd(const char *fdname, Error **errp)
 {
-    const char *fdname = qdict_get_str(qdict, "fdname");
     mon_fd_t *monfd;
     int fd;
 
-    fd = qemu_chr_fe_get_msgfd(mon->chr);
+    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
     if (fd == -1) {
-        qerror_report(QERR_FD_NOT_SUPPLIED);
-        return -1;
+        error_set(errp, QERR_FD_NOT_SUPPLIED);
+        return;
     }
 
     if (qemu_isdigit(fdname[0])) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
-                      "a name not starting with a digit");
-        return -1;
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
+                  "a name not starting with a digit");
+        return;
     }
 
-    QLIST_FOREACH(monfd, &mon->fds, next) {
+    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
 
         close(monfd->fd);
         monfd->fd = fd;
-        return 0;
+        return;
     }
 
     monfd = g_malloc0(sizeof(mon_fd_t));
     monfd->name = g_strdup(fdname);
     monfd->fd = fd;
 
-    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
-    return 0;
+    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+    return;
 }
 
-static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_closefd(const char *fdname, Error **errp)
 {
-    const char *fdname = qdict_get_str(qdict, "fdname");
     mon_fd_t *monfd;
 
-    QLIST_FOREACH(monfd, &mon->fds, next) {
+    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
@@ -2232,11 +2230,11 @@  static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
         close(monfd->fd);
         g_free(monfd->name);
         g_free(monfd);
-        return 0;
+        return;
     }
 
-    qerror_report(QERR_FD_NOT_FOUND, fdname);
-    return -1;
+    error_set(errp, QERR_FD_NOT_FOUND, fdname);
+    return;
 }
 
 static void do_loadvm(Monitor *mon, const QDict *qdict)
diff --git a/qapi-schema.json b/qapi-schema.json
index 3b6e346..6be1d90 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1862,3 +1862,32 @@ 
 # Since: 0.14.0
 ##
 { 'command': 'netdev_del', 'data': {'id': 'str'} }
+
+##
+# @getfd:
+#
+# Receive a file descriptor via SCM rights and assign it a name
+#
+# @fdname: file descriptor name
+#
+# Returns: Nothing on success
+#          If file descriptor was not received, FdNotSupplied
+#          If @fdname is not valid, InvalidParameterType
+#
+# Since: 0.14.0
+##
+{ 'command': 'getfd', 'data': {'fdname': 'str'} }
+
+##
+# @closefd:
+#
+# Close a file descriptor previously passed via SCM rights
+#
+# @fdname: file descriptor name
+#
+# Returns: Nothing on success
+#          If @fdname is not found, FdNotFound
+#
+# Since: 0.14.0
+##
+{ 'command': 'closefd', 'data': {'fdname': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e1a38e..f8c0f68 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -873,8 +873,7 @@  EQMP
         .args_type  = "fdname:s",
         .params     = "getfd name",
         .help       = "receive a file descriptor via SCM rights and assign it a name",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_getfd,
+        .mhandler.cmd_new = qmp_marshal_input_getfd,
     },
 
 SQMP
@@ -899,8 +898,7 @@  EQMP
         .args_type  = "fdname:s",
         .params     = "closefd name",
         .help       = "close a file descriptor previously passed via SCM rights",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_closefd,
+        .mhandler.cmd_new = qmp_marshal_input_closefd,
     },
 
 SQMP