From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlqqG-0006Ei-Cx for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:05:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZlqqB-0007W8-BG for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:05:36 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:33002) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlqqB-0007W1-4P for qemu-devel@nongnu.org; Tue, 13 Oct 2015 00:05:31 -0400 Received: from localhost by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 12 Oct 2015 22:05:30 -0600 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 500783E4003F for ; Mon, 12 Oct 2015 22:05:27 -0600 (MDT) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t9D45RNA63635552 for ; Mon, 12 Oct 2015 21:05:27 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t9D45QOs014481 for ; Mon, 12 Oct 2015 22:05:26 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1444213941-11128-6-git-send-email-den@openvz.org> References: <1444213941-11128-1-git-send-email-den@openvz.org> <1444213941-11128-6-git-send-email-den@openvz.org> Message-ID: <20151013040508.10003.13913@loki> Date: Mon, 12 Oct 2015 23:05:08 -0500 Subject: Re: [Qemu-devel] [PATCH 5/5] qga: guest-exec simple stdin/stdout/stderr redirection List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Yuri Pudgorodskiy , qemu-devel@nongnu.org, v.tolstov@selfip.ru Quoting Denis V. Lunev (2015-10-07 05:32:21) > From: Yuri Pudgorodskiy > = > Implemented with base64-encoded strings in qga json protocol. > Glib portable GIOChannel is used for data I/O. > = > Optinal stdin parameter of guest-exec command is now used as > stdin content for spawned subprocess. > = > If capture-output bool flag is specified, guest-exec redirects out/err > file descriptiors internally to pipes and collects subprocess > output. > = > Guest-exe-status is modified to return this collected data to requestor > in base64 encoding. > = > Signed-off-by: Yuri Pudgorodskiy > Signed-off-by: Denis V. Lunev > CC: Michael Roth For capture-output mode, win32 guests appear to spin forever on EOF and the exec'd process never gets reaped as a result. The below patch seems to fix this issue. If this seems reasonable I can squash it into the patch directly, but you might want to check I didn't break one of your !capture-output use-cases (I assume this is still mainly focused on redirecting to virtio-serial for stdio). Another issue I noticed is that glib relies on gspawn-win{32,64}-helper[-console].exe for g_spawn*() interface, so guest exec won't work for .msi distributables unless those are added. This can probably be addressed during soft-freeze though. diff --git a/qga/commands.c b/qga/commands.c index 27c06c5..fbf8abd 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -318,7 +318,7 @@ static gboolean guest_exec_output_watch(GIOChannel *ch, struct GuestExecIOData *p =3D (struct GuestExecIOData *)p_; gsize bytes_read =3D 0; = - if (cond =3D=3D G_IO_HUP) { + if (cond =3D=3D G_IO_HUP || cond =3D=3D G_IO_ERR) { g_io_channel_unref(ch); g_atomic_int_set(&p->closed, 1); return FALSE; @@ -330,10 +330,18 @@ static gboolean guest_exec_output_watch(GIOChannel *c= h, t =3D g_try_realloc(p->data, p->size + GUEST_EXEC_IO_SIZE); } if (t =3D=3D NULL) { + GIOStatus gstatus =3D 0; p->truncated =3D true; /* ignore truncated output */ gchar buf[GUEST_EXEC_IO_SIZE]; - g_io_channel_read_chars(ch, buf, sizeof(buf), &bytes_read, NUL= L); + gstatus =3D g_io_channel_read_chars(ch, buf, sizeof(buf), + &bytes_read, NULL); + if (gstatus =3D=3D G_IO_STATUS_EOF || gstatus =3D=3D G_IO_STAT= US_ERROR) { + g_io_channel_unref(ch); + g_atomic_int_set(&p->closed, 1); + return false; + } + return TRUE; } p->size +=3D GUEST_EXEC_IO_SIZE; @@ -342,8 +350,14 @@ static gboolean guest_exec_output_watch(GIOChannel *ch, = /* Calling read API once. * On next available data our callback will be called again */ - g_io_channel_read_chars(ch, (gchar *)p->data + p->length, + GIOStatus gstatus =3D 0; + gstatus =3D g_io_channel_read_chars(ch, (gchar *)p->data + p->length, p->size - p->length, &bytes_read, NULL); + if (gstatus =3D=3D G_IO_STATUS_EOF || gstatus =3D=3D G_IO_STATUS_ERROR= ) { + g_io_channel_unref(ch); + g_atomic_int_set(&p->closed, 1); + return false; + } > --- > qga/commands.c | 175 +++++++++++++++++++++++++++++++++++++++++++++= +++--- > qga/qapi-schema.json | 11 +++- > 2 files changed, 175 insertions(+), 11 deletions(-) > = > diff --git a/qga/commands.c b/qga/commands.c > index b487324..740423f 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -15,6 +15,11 @@ > #include "qga-qmp-commands.h" > #include "qapi/qmp/qerror.h" > = > +/* Maximum captured guest-exec out_data/err_data - 16MB */ > +#define GUEST_EXEC_MAX_OUTPUT (16*1024*1024) > +/* Allocation and I/O buffer for reading guest-exec out_data/err_data - = 4KB */ > +#define GUEST_EXEC_IO_SIZE (4*1024) > + > /* Note: in some situations, like with the fsfreeze, logging may be > * temporarilly disabled. if it is necessary that a command be able > * to log for accounting purposes, check ga_logging_enabled() beforehand, > @@ -71,10 +76,23 @@ struct GuestAgentInfo *qmp_guest_info(Error **errp) > return info; > } > = > +struct GuestExecIOData { > + guchar *data; > + gsize size; > + gsize length; > + gint closed; > + bool truncated; > + const char *name; > +}; > + > struct GuestExecInfo { > GPid pid; > gint status; > - bool finished; > + bool has_output; > + gint finished; > + struct GuestExecIOData in; > + struct GuestExecIOData out; > + struct GuestExecIOData err; > QTAILQ_ENTRY(GuestExecInfo) next; > }; > typedef struct GuestExecInfo GuestExecInfo; > @@ -123,9 +141,18 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, = Error **err) > } > = > ges =3D g_new0(GuestExecStatus, 1); > - ges->exited =3D gei->finished; > = > - if (gei->finished) { > + bool finished =3D g_atomic_int_get(&gei->finished); > + > + /* need to wait till output channels are closed > + * to be sure we captured all output at this point */ > + if (gei->has_output) { > + finished =3D finished && g_atomic_int_get(&gei->out.closed); > + finished =3D finished && g_atomic_int_get(&gei->err.closed); > + } > + > + ges->exited =3D finished; > + if (finished) { > /* Glib has no portable way to parse exit status. > * On UNIX, we can get either exit code from normal termination > * or signal number. > @@ -160,6 +187,20 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, = Error **err) > ges->signal =3D WTERMSIG(gei->status); > } > #endif > + if (gei->out.length > 0) { > + ges->has_out_data =3D true; > + ges->out_data =3D g_base64_encode(gei->out.data, gei->out.le= ngth); > + g_free(gei->out.data); > + ges->has_out_truncated =3D gei->out.truncated; > + } > + > + if (gei->err.length > 0) { > + ges->has_err_data =3D true; > + ges->err_data =3D g_base64_encode(gei->err.data, gei->err.le= ngth); > + g_free(gei->err.data); > + ges->has_err_truncated =3D gei->err.truncated; > + } > + > QTAILQ_REMOVE(&guest_exec_state.processes, gei, next); > g_free(gei); > } > @@ -230,6 +271,85 @@ static void guest_exec_task_setup(gpointer data) > #endif > } > = > +static gboolean guest_exec_input_watch(GIOChannel *ch, > + GIOCondition cond, gpointer p_) > +{ > + struct GuestExecIOData *p =3D (struct GuestExecIOData *)p_; > + gsize bytes_written =3D 0; > + GIOStatus status; > + GError *gerr =3D NULL; > + > + /* nothing left to write */ > + if (p->size =3D=3D p->length) { > + goto done; > + } > + > + status =3D g_io_channel_write_chars(ch, (gchar *)p->data + p->length, > + p->size - p->length, &bytes_written, &gerr); > + > + /* can be not 0 even if not G_IO_STATUS_NORMAL */ > + if (bytes_written !=3D 0) { > + p->length +=3D bytes_written; > + } > + > + /* continue write, our callback will be called again */ > + if (status =3D=3D G_IO_STATUS_NORMAL || status =3D=3D G_IO_STATUS_AG= AIN) { > + return TRUE; > + } > + > + if (gerr) { > + g_warning("qga: i/o error writing to inp_data channel: %s", > + gerr->message); > + g_error_free(gerr); > + } > + > +done: > + g_io_channel_shutdown(ch, true, NULL); > + g_io_channel_unref(ch); > + g_atomic_int_set(&p->closed, 1); > + g_free(p->data); > + > + return FALSE; > +} > + > +static gboolean guest_exec_output_watch(GIOChannel *ch, > + GIOCondition cond, gpointer p_) > +{ > + struct GuestExecIOData *p =3D (struct GuestExecIOData *)p_; > + gsize bytes_read =3D 0; > + > + if (cond =3D=3D G_IO_HUP) { > + g_io_channel_unref(ch); > + g_atomic_int_set(&p->closed, 1); > + return FALSE; > + } > + > + if (p->size =3D=3D p->length) { > + gpointer t =3D NULL; > + if (!p->truncated && p->size < GUEST_EXEC_MAX_OUTPUT) { > + t =3D g_try_realloc(p->data, p->size + GUEST_EXEC_IO_SIZE); > + } > + if (t =3D=3D NULL) { > + p->truncated =3D true; > + /* ignore truncated output */ > + gchar buf[GUEST_EXEC_IO_SIZE]; > + g_io_channel_read_chars(ch, buf, sizeof(buf), &bytes_read, N= ULL); > + return TRUE; > + } > + p->size +=3D GUEST_EXEC_IO_SIZE; > + p->data =3D t; > + } > + > + /* Calling read API once. > + * On next available data our callback will be called again */ > + g_io_channel_read_chars(ch, (gchar *)p->data + p->length, > + p->size - p->length, &bytes_read, NULL); > + > + p->length +=3D bytes_read; > + > + return TRUE; > +} > + > GuestExec *qmp_guest_exec(const char *path, > bool has_arg, strList *arg, > bool has_env, strList *env, > @@ -244,6 +364,10 @@ GuestExec *qmp_guest_exec(const char *path, > strList arglist; > gboolean ret; > GError *gerr =3D NULL; > + gint in_fd, out_fd, err_fd; > + GIOChannel *in_ch, *out_ch, *err_ch; > + GSpawnFlags flags; > + bool has_output =3D (has_capture_output && capture_output); > = > arglist.value =3D (char *)path; > arglist.next =3D has_arg ? arg : NULL; > @@ -251,11 +375,14 @@ GuestExec *qmp_guest_exec(const char *path, > argv =3D guest_exec_get_args(&arglist, true); > envp =3D guest_exec_get_args(has_env ? env : NULL, false); > = > - ret =3D g_spawn_async_with_pipes(NULL, argv, envp, > - G_SPAWN_SEARCH_PATH | > - G_SPAWN_DO_NOT_REAP_CHILD | > - G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL, > - guest_exec_task_setup, NULL, &pid, NULL, NULL, NULL, &gerr); > + flags =3D G_SPAWN_SEARCH_PATH | G_SPAWN_DO_NOT_REAP_CHILD; > + if (!has_output) { > + flags |=3D G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NU= LL; > + } > + > + ret =3D g_spawn_async_with_pipes(NULL, argv, envp, flags, > + guest_exec_task_setup, NULL, &pid, has_inp_data ? &in_fd : N= ULL, > + has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &g= err); > if (!ret) { > error_setg(err, QERR_QGA_COMMAND_FAILED, gerr->message); > g_error_free(gerr); > @@ -266,8 +393,40 @@ GuestExec *qmp_guest_exec(const char *path, > ge->pid =3D (int64_t)pid; > = > gei =3D guest_exec_info_add(pid); > + gei->has_output =3D has_output; > g_child_watch_add(pid, guest_exec_child_watch, gei); > = > + if (has_inp_data) { > + gei->in.data =3D g_base64_decode(inp_data, &gei->in.size); > +#ifdef G_OS_WIN32 > + in_ch =3D g_io_channel_win32_new_fd(in_ch); > +#else > + in_ch =3D g_io_channel_unix_new(in_fd); > +#endif > + g_io_channel_set_encoding(in_ch, NULL, NULL); > + g_io_channel_set_buffered(in_ch, false); > + g_io_channel_set_flags(in_ch, G_IO_FLAG_NONBLOCK, NULL); > + g_io_add_watch(in_ch, G_IO_OUT, guest_exec_input_watch, &gei->in= ); > + } > + > + if (has_output) { > +#ifdef G_OS_WIN32 > + out_ch =3D g_io_channel_win32_new_fd(out_fd); > + err_ch =3D g_io_channel_win32_new_fd(err_fd); > +#else > + out_ch =3D g_io_channel_unix_new(out_fd); > + err_ch =3D g_io_channel_unix_new(err_fd); > +#endif > + g_io_channel_set_encoding(out_ch, NULL, NULL); > + g_io_channel_set_encoding(err_ch, NULL, NULL); > + g_io_channel_set_buffered(out_ch, false); > + g_io_channel_set_buffered(err_ch, false); > + g_io_add_watch(out_ch, G_IO_IN | G_IO_HUP, > + guest_exec_output_watch, &gei->out); > + g_io_add_watch(err_ch, G_IO_IN | G_IO_HUP, > + guest_exec_output_watch, &gei->err); > + } > + > done: > g_free(argv); > g_free(envp); > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index da6b64f..6f5ea50 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -941,12 +941,17 @@ > # @err-data: #optional base64-encoded stderr of the process > # Note: @out-data and @err-data are present only > # if 'capture-output' was specified for 'guest-exec' > +# @out-truncated: #optional true if stdout was not fully captured > +# due to size limitation. > +# @err-truncated: #optional true if stderr was not fully captured > +# due to size limitation. > # > # Since: 2.5 > ## > { 'struct': 'GuestExecStatus', > 'data': { 'exited': 'bool', '*exitcode': 'int', '*signal': 'int', > - '*out-data': 'str', '*err-data': 'str' }} > + '*out-data': 'str', '*err-data': 'str', > + '*out-truncated': 'bool', '*err-truncated': 'bool' }} > ## > # @guest-exec-status > # > @@ -955,9 +960,9 @@ > # > # @pid: pid returned from guest-exec > # > -# Returns: @GuestExecStatus on success. > +# Returns: GuestExecStatus on success. > # > -# Since 2.3 > +# Since 2.5 > ## > { 'command': 'guest-exec-status', > 'data': { 'pid': 'int' }, > -- = > 2.1.4 >=20