| Message ID | 1434733075-24240-3-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
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? > + > +## > +# @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 >
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(-) > > + > + if (pipe(fd) != 0) { > + error_set_errno(errp, errno, QERR_QGA_COMMAND_FAILED, "pipe() failed"); > + return NULL; > + } > + > + this_end = (mode == GUEST_PIPE_MODE_WRITE); > + other_end = !this_end; > + > + qemu_set_nonblock(fd[this_end]); > + > + qemu_set_cloexec(fd[this_end]); > + qemu_set_cloexec(fd[other_end]); Would it be better to create a named FIFO somewhere in the file system, so that you can reopen the connection even if the qga daemon is restarted? By using just pipe(), your fds are rather ephemeral, but if I understand correctly, the rest of the guest-file API tries to persist across qga restart.
On 19/06/15 20:34, 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(-) >> >> + >> + if (pipe(fd) != 0) { >> + error_set_errno(errp, errno, QERR_QGA_COMMAND_FAILED, "pipe() failed"); >> + return NULL; >> + } >> + >> + this_end = (mode == GUEST_PIPE_MODE_WRITE); >> + other_end = !this_end; >> + >> + qemu_set_nonblock(fd[this_end]); >> + >> + qemu_set_cloexec(fd[this_end]); >> + qemu_set_cloexec(fd[other_end]); > Would it be better to create a named FIFO somewhere in the file system, > so that you can reopen the connection even if the qga daemon is > restarted? By using just pipe(), your fds are rather ephemeral, but if > I understand correctly, the rest of the guest-file API tries to persist > across qga restart. > this will not help IMHO Let us assume that we have a FIFO on a filesystem and we have a process which uses this FIFO as stdout. If QGA will exit that process will blown up with SIGPIPE on any write attempt. Den
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 >>
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? > >> + >> +## >> +# @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
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index d0f371b..a616996 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -212,6 +212,7 @@ 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; @@ -219,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; @@ -232,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; @@ -399,7 +402,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, */ qemu_set_nonblock(fileno(fh)); - handle = guest_file_handle_add(fh, errp); + handle = guest_file_handle_add(fh, -1, errp); if (handle < 0) { fclose(fh); return -1; @@ -409,6 +412,85 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, return handle; } +GuestPipeInfoList *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp) +{ + FILE *f = NULL; + int fd[2], this_end, other_end; + int64_t handle; + GuestPipeInfoList *pipe_list; + GuestPipeInfo *pipe_inf; + const char *op_flag; + + slog("guest-pipe-open called"); + + if (mode > 2) { + error_set(errp, ERROR_CLASS_GENERIC_ERROR, + "Only \"r\" or \"w\" are the valid modes to open a pipe"); + return NULL; + } + + if (pipe(fd) != 0) { + error_set_errno(errp, errno, QERR_QGA_COMMAND_FAILED, "pipe() failed"); + return NULL; + } + + this_end = (mode == GUEST_PIPE_MODE_WRITE); + other_end = !this_end; + + qemu_set_nonblock(fd[this_end]); + + qemu_set_cloexec(fd[this_end]); + qemu_set_cloexec(fd[other_end]); + + if (mode == GUEST_PIPE_MODE_READ) { + op_flag = "r"; + } else { + op_flag = "w"; + } + f = fdopen(fd[this_end], op_flag); + if (f == NULL) { + error_set_errno(errp, errno, QERR_QGA_COMMAND_FAILED, + "fdopen() failed to open pipe handle"); + goto fail; + } + + handle = guest_file_handle_add(f, fd[other_end], errp); + if (handle == -1) { + goto fail; + } + + slog("guest-pipe-open: handle: %" PRId64, handle); + + pipe_inf = g_malloc0(sizeof(*pipe_inf)); + pipe_inf->fd = handle; + pipe_list = g_malloc0(sizeof(*pipe_list)); + pipe_list->value = pipe_inf; + pipe_list->next = NULL; + return pipe_list; + +fail: + if (f != NULL) { + fclose(f); + } else { + close(fd[this_end]); + } + close(fd[other_end]); + return NULL; +} + +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); @@ -419,6 +501,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"); @@ -2395,7 +2482,7 @@ GList *ga_command_blacklist_init(GList *blacklist) "guest-suspend-hybrid", "guest-network-get-interfaces", "guest-get-vcpus", "guest-set-vcpus", "guest-get-memory-blocks", "guest-set-memory-blocks", - "guest-get-memory-block-size", NULL}; + "guest-get-memory-block-size", "guest-pipe-open", NULL}; char **p = (char **)list; while (*p) { @@ -2409,7 +2496,8 @@ GList *ga_command_blacklist_init(GList *blacklist) const char *list[] = { "guest-get-fsinfo", "guest-fsfreeze-status", "guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list", - "guest-fsfreeze-thaw", "guest-get-fsinfo", NULL}; + "guest-fsfreeze-thaw", "guest-get-fsinfo", + "guest-pipe-open", NULL}; char **p = (char **)list; while (*p) { diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 3ef0549..685dd0f 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; } +GuestPipeInfoList *qmp_guest_pipe_open(GuestPipeMode mode, Error **errp) +{ + error_set(errp, QERR_UNSUPPORTED); + return NULL; +} + void qmp_guest_file_close(int64_t handle, Error **errp) { bool ret; @@ -713,7 +719,7 @@ GList *ga_command_blacklist_init(GList *blacklist) "guest-get-memory-blocks", "guest-set-memory-blocks", "guest-get-memory-block-size", "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 b446dc7..8081213 100644 --- a/qga/qapi-schema.json +++ 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'} } + +## +# @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 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'] } + +## +## # @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