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

["Denis V. Lunev" <den@openvz.org>]

On 19/06/15 20:30, Eric Blake wrote:
> On 06/19/2015 10:57 AM, Denis V. Lunev wrote:
>> From: Olga Krishtal <okrishtal@virtuozzo.com>
>>
>> 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 <okrishtal@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Acked-by: Roman Kagan <rkagan@virtuozzo.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>   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
>>