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

[Eric Blake]

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 } ] ?

> +
> +##
> +##
>  # @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
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature