Re: [Qemu-devel] [PATCH 02/10] qga: implement guest-pipe-open command

[Denis V. Lunev]

On 19/06/15 20:30, Eric Blake wrote:
> On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
> > From: Olga Krishtal <address@hidden>
> >
> > The command creates FIFO pair that can be used with existing file
> > read/write interfaces to communicate with processes spawned via the
> > forthcoming guest-file-exec interface.
> >
> > Signed-off-by: Olga Krishtal <address@hidden>
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > Acked-by: Roman Kagan <address@hidden>
> > CC: Eric Blake <address@hidden>
> > CC: Michael Roth <address@hidden>
> > ---
> >   qga/commands-posix.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >   qga/commands-win32.c |  8 ++++-
> >   qga/qapi-schema.json | 44 ++++++++++++++++++++++++
> >   3 files changed, 143 insertions(+), 5 deletions(-)
> Just an interface review at this point:
>
> > +++ b/qga/qapi-schema.json
> > @@ -215,12 +215,56 @@
> >     'returns': 'int' }
> >   
> >   ##
> > +# @GuestPipeMode
> > +#
> > +# An enumeration of pipe modes
> > +# read: pipe is opened for writing data
> > +# write: pipe is opened for reading data
> > +#
> > +# Since: 2.4
> > +##
> > +{ 'enum': 'GuestPipeMode',
> > +  'data': [ 'read', 'write' ] }
> > +
> > +##
> > +# @GuestPipeInfo
> > +#
> > +# Information about pipe.
> > +#
> > +# Since: 2.4
> > +##
> > +{ 'struct': 'GuestPipeInfo',
> > +  'data': {'fd': 'int'} }
> Missing a field of type GuestPipeMode?
yep, nice catch :)

> > +
> > +##
> > +# @guest-pipe-open
> > +#
> > +# Open a pipe to in the guest to associated with a qga-spawned processes
> > +# for communication.
> Reads poorly.  Maybe:
>
> Open a pipe in the guest for association with later qga-spawned processes.
ok

> > +#
> > +# Returns: Guest file handle on success, as per guest-file-open. This
> > +# handle is usable with the same interfaces as a handle returned by
> > +# guest-file-open.
> > +#
> > +# Since: 2.4
> > +##
> > +{ 'command': 'guest-pipe-open',
> > +  'data':    { 'mode': 'GuestPipeMode' },
> > +  'returns': ['GuestPipeInfo'] }
> I'm assuming the array will always contain two elements?  Would it be
> any simpler to return a single dictionary of { 'read': 'int', 'write':
> 'int' } for naming the two fds directly, instead of having to parse
> through [ { 'fd': 1 }, { 'fd': 2 } ] ?

this looks much better to me

> > +
> > +##
> > +##
> >   # @guest-file-close:
> >   #
> >   # Close an open file in the guest
> >   #
> >   # @handle: filehandle returned by guest-file-open
> >   #
> > +# Please note that closing the write side of a pipe will block until the read
> > +# side is closed.  If you passed the read-side of the pipe to a qga-spawned
> > +# process, make sure the process has exited before attempting to close the
> > +# write side.
> > +#
> >   # Returns: Nothing on success.
> >   #
> >   # Since: 0.15.0