Re: [PATCH v4 11/11] QMP/HMP: only actually implement getfd on CONFIG_POSIX

[Markus Armbruster <armbru@redhat.com>]

Regarding the subject line:

1. We use "qmp:" for qmp-only patches, "hmp:" for hmp-only patches, and
"monitor:" for both.  "qmp hmp:" would work there, too.

2. We're not implementing anything, we're restricting the existing
implementation to hosts where it is actually useful.

Suggest "monitor: Restrict command getfd to POSIX hosts"

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Currently, the function will simply fail if ancillary fds are not
> provided, for ex on unsupported platforms.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/misc.json     | 2 +-
>  monitor/fds.c      | 2 ++
>  monitor/hmp-cmds.c | 2 ++
>  hmp-commands.hx    | 2 ++
>  4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 031c94050c..96c053e305 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -273,7 +273,7 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'getfd', 'data': {'fdname': 'str'} }
> +{ 'command': 'getfd', 'data': {'fdname': 'str'}, 'if': 'CONFIG_POSIX' }
>  
>  ##
>  # @get-win32-socket:

This changes the failure from

    {"error": {"class": "GenericError", "desc": "No file descriptor supplied via SCM_RIGHTS"}}

to

    {"error": {"class": "CommandNotFound", "desc": "The command getfd has not been found"}}

when CONFIG_POSIX is off.  I think this is fine.  But I'd like the
commit message to document it.


> diff --git a/monitor/fds.c b/monitor/fds.c
> index 9ed4197358..d86c2c674c 100644
> --- a/monitor/fds.c
> +++ b/monitor/fds.c
> @@ -98,6 +98,7 @@ static bool monitor_add_fd(Monitor *mon, int fd, const char *fdname, Error **err
>      return true;
>  }
>  
> +#ifdef CONFIG_POSIX
>  void qmp_getfd(const char *fdname, Error **errp)
>  {
>      Monitor *cur_mon = monitor_cur();
> @@ -111,6 +112,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>  
>      monitor_add_fd(cur_mon, fd, fdname, errp);
>  }
> +#endif
>  
>  void qmp_closefd(const char *fdname, Error **errp)
>  {
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 34bd8c67d7..6c559b48c8 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -192,6 +192,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, err);
>  }
>  
> +#ifdef CONFIG_POSIX
>  void hmp_getfd(Monitor *mon, const QDict *qdict)
>  {
>      const char *fdname = qdict_get_str(qdict, "fdname");
> @@ -200,6 +201,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
>      qmp_getfd(fdname, &err);
>      hmp_handle_error(mon, err);
>  }
> +#endif
>  
>  void hmp_closefd(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index b87c250e23..bb85ee1d26 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1486,6 +1486,7 @@ SRST
>    Inject an MCE on the given CPU (x86 only).
>  ERST
>  
> +#ifdef CONFIG_POSIX
>      {
>          .name       = "getfd",
>          .args_type  = "fdname:s",
> @@ -1501,6 +1502,7 @@ SRST
>    mechanism on unix sockets, it is stored using the name *fdname* for
>    later use by other monitor commands.
>  ERST
> +#endif
>  
>      {
>          .name       = "closefd",

With the commit message brushed up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>