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?
>
> > +
> > +##
> > +# @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.
>
> > +#
> > +# 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 } ] ?

Eric,

I have mistaken here in the previous letter. The idea
is that we return here only one int, which is qemu file
handle representing the pipe as an entity.

May be this should be declared in a different way to
allow future extendability. Can you advise on this?

Regards,
    Den