[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command



On Fri, 15 Jun 2012 11:04:16 -0400
Corey Bryant <coreyb linux vnet ibm com> wrote:

> 
> 
> On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
> > On Thu, 14 Jun 2012 11:55:02 -0400
> > Corey Bryant <coreyb linux vnet ibm com> wrote:
> >
> >> This patch adds the pass-fd QMP command using the QAPI framework.
> >> Like the getfd command, it is used to pass a file descriptor via
> >> SCM_RIGHTS.  However, the pass-fd command also returns the received
> >> file descriptor, which is a difference in behavior from the getfd
> >> command, which returns nothing.
> >>
> >> The closefd command can be used to close a file descriptor that was
> >> passed with the pass-fd command.
> >>
> >> Note that when using getfd or pass-fd, there are some commands
> >> (e.g. migrate with fd:name) that implicitly close the named fd.
> >> When this is not the case, closefd must be used to avoid fd leaks.
> >>
> >> Signed-off-by: Corey Bryant <coreyb linux vnet ibm com>
> >> ---
> >> v2:
> >>   -Introduce new QMP command to pass/return fd (lcapitulino redhat com)
> >>   -Use passfd as command name (berrange redhat com)
> >>
> >> v3:
> >>   -Use pass-fd as command name (lcapitulino redhat com)
> >>   -Fail pass-fd if fdname already exists (lcapitulino redhat com)
> >>   -Add notes to QMP command describing behavior in more detail
> >>    (lcapitulino redhat com, eblake redhat com)
> >>   -Add note about fd leakage (eblake redhat com)
> >>
> >>   monitor.c        |   33 +++++++++++++++++++++++++++++++++
> >>   qapi-schema.json |   19 +++++++++++++++++++
> >>   qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
> >>   3 files changed, 86 insertions(+)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 1a7f7e7..6d99368 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
> >>   }
> >>   #endif
> >>
> >> +int64_t qmp_pass_fd(const char *fdname, Error **errp)
> >> +{
> >> +    mon_fd_t *monfd;
> >> +    int fd;
> >> +
> >> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
> >> +    if (fd == -1) {
> >> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (qemu_isdigit(fdname[0])) {
> >> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> >> +                  "a name not starting with a digit");
> >> +        return -1;
> >> +    }
> >> +
> >> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >> +        if (strcmp(monfd->name, fdname) == 0) {
> >> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> >> +                      "a name that does not already exist");
> >> +            return -1;
> >> +        }
> >> +    }
> >
> > Returning the same error class for two different errors is not a good idea.
> > I think you have two options here. You could return QERR_INVALID_PARAMETER
> > for the "already exists" case or introduce QERR_FD_EXISTS. The later is
> > certainly nicer, but we were trying to avoid having too specific errors...
> >
> 
> I'm not clear on what the problem is with returning the same error class 
> for two different errors.  Could you explain?  I don't have a problem 
> changing it if it's an issue.

Because an mngt app/user won't know if what has happened was that the fd name
is invalid or if the fd name already exists.

> 
> >> +
> >> +    monfd = g_malloc0(sizeof(mon_fd_t));
> >> +    monfd->name = g_strdup(fdname);
> >> +    monfd->fd = fd;
> >
> > Maybe you could try to move this to a separate function to share code with
> > qmp_getfd()?
> >
> 
> Sure, no problem.  I can do that.
> 
> >> +
> >> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> >> +    return fd;
> >> +}
> >> +
> >>   void qmp_getfd(const char *fdname, Error **errp)
> >>   {
> >>       mon_fd_t *monfd;
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 26a6b84..ed99f23 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -1864,6 +1864,25 @@
> >>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >>
> >>   ##
> >> +# @pass-fd:
> >> +#
> >> +# Pass a file descriptor via SCM rights and assign it a name
> >> +#
> >> +# @fdname: file descriptor name
> >> +#
> >> +# Returns: The QEMU file descriptor that was received
> >> +#          If file descriptor was not received, FdNotSupplied
> >> +#          If @fdname is not valid, InvalidParameterType
> >> +#
> >> +# Since: 1.2.0
> >> +#
> >> +# Notes: If @fdname already exists, the command will fail.
> >> +#        The 'closefd' command can be used to explicitly close the
> >> +#        file descriptor when it is no longer needed.
> >> +##
> >> +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
> >> +
> >> +##
> >>   # @getfd:
> >>   #
> >>   # Receive a file descriptor via SCM rights and assign it a name
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index e3cf3c5..c039947 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -869,6 +869,40 @@ Example:
> >>   EQMP
> >>
> >>       {
> >> +        .name       = "pass-fd",
> >> +        .args_type  = "fdname:s",
> >> +        .params     = "pass-fd name",
> >> +        .help       = "pass a file descriptor via SCM rights and assign it a name",
> >> +        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
> >> +    },
> >> +
> >> +SQMP
> >> +pass-fd
> >> +-------
> >> +
> >> +Pass a file descriptor via SCM rights and assign it a name.
> >> +
> >> +Arguments:
> >> +
> >> +- "fdname": file descriptor name (json-string)
> >> +
> >> +Return a json-int with the QEMU file descriptor that was received.
> >> +
> >> +Example:
> >> +
> >> +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
> >> +<- { "return": 42 }
> >> +
> >> +Notes:
> >> +
> >> +(1) If the name specified by the "fdname" argument already exists,
> >> +    the command will fail.
> >> +(2) The 'closefd' command can be used to explicitly close the file
> >> +    descriptor when it is no longer needed.
> >> +
> >> +EQMP
> >> +
> >> +    {
> >>           .name       = "getfd",
> >>           .args_type  = "fdname:s",
> >>           .params     = "getfd name",
> >
> >
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]