diff mbox

[4/8] guest agent: add guest-pipe-open

Message ID 1420031214-6053-5-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Dec. 31, 2014, 1:06 p.m. UTC
From: Simon Zolin <szolin@parallels.com>

Creates a 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: Simon Zolin <szolin@parallels.com>
Acked-by: Roman Kagan <rkagan@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 qga/commands-win32.c |  8 ++++-
 qga/qapi-schema.json | 21 ++++++++++++++
 3 files changed, 108 insertions(+), 3 deletions(-)

Comments

Eric Blake Feb. 3, 2015, 9:57 p.m. UTC | #1
On 12/31/2014 06:06 AM, Denis V. Lunev wrote:
> From: Simon Zolin <szolin@parallels.com>
> 
> Creates a 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: Simon Zolin <szolin@parallels.com>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---

> +++ b/qga/qapi-schema.json
> @@ -212,12 +212,33 @@
>    'returns': 'int' }
>  
>  ##
> +# @guest-pipe-open
> +#
> +# Open a pipe to in the guest to associated with a qga-spawned processes
> +# for communication.
> +#
> +# Returns: Guest file handle on success, as per guest-file-open. This
> +# handle is useable with the same interfaces as a handle returned by

s/useable/usable/

> +# guest-file-open.
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'guest-pipe-open',
> +  'data':    { 'mode': 'str' },
> +  'returns': 'int' }

I'm not a fan of returning a bare 'int' - it is not extensible.  Better
is returning a dictionary, such as 'returns': { 'fd': 'int' }.  That
way, if we ever find a reason to return multiple pieces of information,
we just return a larger dictionary.

Yeah, I know guest-pipe-open breaks the rules here, and so consistency
may be an argument in favor of also breaking the rules.

I don't like 'mode' encoded as a raw string.  Make it an enum type (as
in { 'enum':'PipeMode', 'data':['read', 'write']} ... 'mode':'PipeMode')
or even a bool (as in 'read':'bool')

This only returns ONE end of a pipe (good for when the host is piping
data into the child, or when the child is piping data into the host).
But isn't your goal to also make it possible to string together multiple
child processes where the output of one is the input of the other (no
host involvement)?  How would you wire that up?

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

How does one pass the read side of a pipe to a spawned child?  Can you
design the spawning API so that close cannot deadlock?

> +#
>  # Returns: Nothing on success.
>  #
>  # Since: 0.15.0
>
Eric Blake Feb. 3, 2015, 10:06 p.m. UTC | #2
On 02/03/2015 02:57 PM, Eric Blake wrote:

>> +# Returns: Guest file handle on success, as per guest-file-open. This
>> +# handle is useable with the same interfaces as a handle returned by
> 

>> +  'returns': 'int' }
> 
> I'm not a fan of returning a bare 'int' - it is not extensible.  Better
> is returning a dictionary, such as 'returns': { 'fd': 'int' }.  That
> way, if we ever find a reason to return multiple pieces of information,
> we just return a larger dictionary.
> 
> Yeah, I know guest-pipe-open breaks the rules here, and so consistency
> may be an argument in favor of also breaking the rules.

I meant to say 'guest-file-open' breaks the rules, and that you are
proposing that 'guest-pipe-open' be consistent with 'guest-file-open'.
diff mbox

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index fd746db..bf14518 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -208,9 +208,11 @@  void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
     }
 }
 
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
+    int pipe_other_end_fd; /* if set, it's a pipe fd of the other end. */
     QTAILQ_ENTRY(GuestFileHandle) next;
 } GuestFileHandle;
 
@@ -218,7 +220,8 @@  static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
 } guest_file_state;
 
-static int64_t guest_file_handle_add(FILE *fh, Error **errp)
+static int64_t guest_file_handle_add(FILE *fh, int pipe_other_end_fd,
+                                     Error **errp)
 {
     GuestFileHandle *gfh;
     int64_t handle;
@@ -231,6 +234,7 @@  static int64_t guest_file_handle_add(FILE *fh, Error **errp)
     gfh = g_malloc0(sizeof(GuestFileHandle));
     gfh->id = handle;
     gfh->fh = fh;
+    gfh->pipe_other_end_fd = pipe_other_end_fd;
     QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
 
     return handle;
@@ -422,7 +426,7 @@  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
         return -1;
     }
 
-    handle = guest_file_handle_add(fh, errp);
+    handle = guest_file_handle_add(fh, -1, errp);
     if (handle < 0) {
         fclose(fh);
         return -1;
@@ -432,6 +436,75 @@  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
     return handle;
 }
 
+int64_t qmp_guest_pipe_open(const char *mode, Error **err)
+{
+    FILE *f = NULL;
+    int fd[2], this_end, other_end;
+    int64_t handle;
+
+    slog("guest-pipe-open called");
+
+    if ((mode[0] != 'r' && mode[0] != 'w') || mode[1] != '\0') {
+        error_set(err, ERROR_CLASS_GENERIC_ERROR,
+                  "Only \"r\" or \"w\" are the valid modes to open a pipe");
+        return -1;
+    }
+
+    if (pipe(fd) != 0) {
+        error_set_errno(err, errno, QERR_QGA_COMMAND_FAILED, "pipe() failed");
+        return -1;
+    }
+
+    this_end = (mode[0] == 'w');
+    other_end = !this_end;
+
+    if (guest_file_toggle_flags(fd[this_end], O_NONBLOCK, true, err) == -1) {
+        goto fail;
+    }
+
+    qemu_set_cloexec(fd[this_end]);
+    qemu_set_cloexec(fd[other_end]);
+
+    f = fdopen(fd[this_end], mode);
+    if (f == NULL) {
+        error_set_errno(err, errno, QERR_QGA_COMMAND_FAILED,
+                        "fdopen() failed to open pipe handle");
+        goto fail;
+    }
+
+    handle = guest_file_handle_add(f, fd[other_end], err);
+    if (handle == -1) {
+        goto fail;
+    }
+
+    slog("guest-pipe-open: handle: %" PRId64, handle);
+
+    return handle;
+
+fail:
+    if (f != NULL) {
+        fclose(f);
+    } else {
+        close(fd[this_end]);
+    }
+    close(fd[other_end]);
+
+    return -1;
+}
+
+static int guest_pipe_close_other_end(GuestFileHandle *gfh)
+{
+    if (gfh->pipe_other_end_fd != -1) {
+        if (close(gfh->pipe_other_end_fd) != 0) {
+            return 1;
+        }
+
+        gfh->pipe_other_end_fd = -1;
+    }
+
+    return 0;
+}
+
 void qmp_guest_file_close(int64_t handle, Error **errp)
 {
     GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
@@ -442,6 +515,11 @@  void qmp_guest_file_close(int64_t handle, Error **errp)
         return;
     }
 
+    if (guest_pipe_close_other_end(gfh) != 0) {
+        error_setg_errno(errp, errno, "failed to close pipe handle");
+        return;
+    }
+
     ret = fclose(gfh->fh);
     if (ret == EOF) {
         error_setg_errno(errp, errno, "failed to close handle");
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 5db96ef..5cb7946 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -154,6 +154,12 @@  int64_t qmp_guest_file_open(const char *path, bool has_mode,
     return fd;
 }
 
+int64_t qmp_guest_pipe_open(const char *mode, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return 0;
+}
+
 void qmp_guest_file_close(int64_t handle, Error **errp)
 {
     bool ret;
@@ -683,7 +689,7 @@  GList *ga_command_blacklist_init(GList *blacklist)
         "guest-suspend-hybrid", "guest-network-get-interfaces",
         "guest-get-vcpus", "guest-set-vcpus",
         "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
-        "guest-fstrim", NULL};
+        "guest-fstrim", "guest-pipe-open", NULL};
     char **p = (char **)list_unsupported;
 
     while (*p) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 376e79f..67d3b72 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -212,12 +212,33 @@ 
   'returns': 'int' }
 
 ##
+# @guest-pipe-open
+#
+# Open a pipe to in the guest to associated with a qga-spawned processes
+# for communication.
+#
+# Returns: Guest file handle on success, as per guest-file-open. This
+# handle is useable with the same interfaces as a handle returned by
+# guest-file-open.
+#
+# Since: 2.3
+##
+{ 'command': 'guest-pipe-open',
+  'data':    { 'mode': 'str' },
+  'returns': 'int' }
+
+##
 # @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