Re: [Qemu-devel] [PATCH 01/12] monitor: use monitor_handle_fd_param for non-Error-friendly users of named fds

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 20 Sep 2012 10:09:54 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 19/09/2012 22:42, Luiz Capitulino ha scritto:
> > On Wed, 19 Sep 2012 16:31:04 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> monitor_handle_fd_param and monitor_get_fd are mostly the same, except
> >> that monitor_handle_fd_param does error reporting wrong.  Use it in all
> >> other places that do it wrong, instead of reinventing it.
> > 
> > Hmm, why do we want to do this?
> > 
> > As far as I understand it the main difference between the two functions
> > is that if fdname is a number (for a weak definition of number),
> > monitor_handle_fd_param() assumes that the fd already exists in qemu
> > (eg. it was passed by the parent process). 
> 
> Oops, right, I remembered it was the other way round (i.e. first search
> for a named file descriptor, and fall back to a numeric one).

That's a lot better, IMO.

> > Another side effect is that you add the possibility of functions
> > changing from monitor_get_fd() to monitor_handle_fd_param() to also take
> > fds passed by the parent process. Might be positive, but I wonder if that's
> > useful for the commands you're changing.
> 
> Making behavior more uniform may be a useful thing on its own.  We might
> even consider moving it to monitor_get_fd (with the above tweak for
> compatibility).

Yes, although I can think of another issue: suppose an mngt app passes an
fd with fdname=9, but the fd doesn't reach qemu for some reason. Then the
mngt app executes a qmp command with fdname=9 and fd 9 turns out to exist
in qemu... Actually, a mngt app could do this even w/o passing an fd to
qemu.

Not sure how relevant this issue is though, as this can happen today
with qmp commands using monitor_handle_fd_param().

> BTW, pci-assign.c is open-coding
> monitor_handle_fd_param (including the numeric file descriptor behavior)
> and we can remove the surrounding if.

Yes, that's fine.