* [PATCH] [v2] libxl: Add API to retrieve domain console tty
@ 2012-05-23 6:35 Bamvor Jian Zhang
2012-05-29 9:54 ` Ian Campbell
2012-05-29 10:16 ` Ian Jackson
0 siblings, 2 replies; 4+ messages in thread
From: Bamvor Jian Zhang @ 2012-05-23 6:35 UTC (permalink / raw)
To: xen-devel; +Cc: jfehlig, Ian.Jackson, Ian.Campbell, bjzhang
This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.
Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
Changes since v1:
* Replace function libxl__sprintf with macro GSPRINTF
* check return value and handle error for libxl__xs_read and libxl__domain_type
* merge common code for libxl_primary_console_get_tty and
libxl_primary_console_exec as libxl__primary_console_find
* Reformat code to be less than 80 characters.
diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Tue May 22 11:55:02 2012 +0100
+++ b/tools/libxl/libxl.c Wed May 23 14:27:57 2012 +0800
@@ -1188,7 +1188,8 @@ out:
return rc;
}
-int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type)
+int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
+ libxl_console_type type)
{
GC_INIT(ctx);
char *p = libxl__sprintf(gc, "%s/xenconsole", libxl__private_bindir_path());
@@ -1214,24 +1215,74 @@ out:
return ERROR_FAIL;
}
-int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
+int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
+ libxl_console_type type, char **path)
+{
+ GC_INIT(ctx);
+ char *dom_path = 0;
+ char *tty_path = 0;
+ char *tty = 0;
+ int rc = 0;
+
+ dom_path = libxl__xs_get_dompath(gc, domid);
+ if (!dom_path) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ switch (type) {
+ case LIBXL_CONSOLE_TYPE_SERIAL:
+ tty_path = GCSPRINTF("%s/serial/0/tty", dom_path);
+ break;
+ case LIBXL_CONSOLE_TYPE_PV:
+ if (cons_num == 0)
+ tty_path = GCSPRINTF("%s/console/tty", dom_path);
+ else
+ tty_path = GCSPRINTF("%s/device/console/%d/tty", dom_path,
+ cons_num);
+ break;
+ default:
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ tty = libxl__xs_read(gc, XBT_NULL, tty_path);
+ if (tty)
+ *path = strdup(tty);
+ else
+ rc = ERROR_FAIL;
+
+out:
+ GC_FREE;
+ return rc;
+}
+
+static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm,
+ uint32_t *domid_out, int *cons_num_out,
+ libxl_console_type *type_out)
{
GC_INIT(ctx);
uint32_t stubdomid = libxl_get_stubdom_id(ctx, domid_vm);
- int rc;
- if (stubdomid)
- rc = libxl_console_exec(ctx, stubdomid,
- STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV);
- else {
+ int rc = 0;
+
+ if (stubdomid) {
+ *domid_out = stubdomid;
+ *cons_num_out = STUBDOM_CONSOLE_SERIAL;
+ *type_out = LIBXL_CONSOLE_TYPE_PV;
+ } else {
switch (libxl__domain_type(gc, domid_vm)) {
case LIBXL_DOMAIN_TYPE_HVM:
- rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_SERIAL);
+ *domid_out = domid_vm;
+ *cons_num_out = 0;
+ *type_out = LIBXL_CONSOLE_TYPE_SERIAL;
break;
case LIBXL_DOMAIN_TYPE_PV:
- rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
+ *domid_out = domid_vm;
+ *cons_num_out = 0;
+ *type_out = LIBXL_CONSOLE_TYPE_PV;
break;
case -1:
- LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
+ LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
rc = ERROR_FAIL;
break;
default:
@@ -1242,6 +1293,33 @@ int libxl_primary_console_exec(libxl_ctx
return rc;
}
+int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
+{
+ uint32_t domid_out;
+ int cons_num_out;
+ libxl_console_type type_out;
+ int rc;
+
+ rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out,
+ &type_out);
+ if ( rc ) return rc;
+ return libxl_console_exec(ctx, domid_out, cons_num_out, type_out);
+}
+
+int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
+ char **path)
+{
+ uint32_t domid_out;
+ int cons_num_out;
+ libxl_console_type type_out;
+ int rc;
+
+ rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out,
+ &type_out);
+ if ( rc ) return rc;
+ return libxl_console_get_tty(ctx, domid_out, cons_num_out, type_out, path);
+}
+
int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
{
GC_INIT(ctx);
diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Tue May 22 11:55:02 2012 +0100
+++ b/tools/libxl/libxl.h Wed May 23 14:27:57 2012 +0800
@@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
* case of HVM guests, and before libxl_run_bootloader in case of PV
* guests using pygrub. */
int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
+/* libxl_console_get_tty retrieves the specified domain's console tty path
+ * and stores it in path. Caller is responsible for freeing the memory.
+ */
+int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
+ libxl_console_type type, char **path);
+/* libxl_primary_console_get_tty retrieves the specified domain's primary
+ * console tty path and stores it in path. Caller is responsible for freeing
+ * the memory.
+ */
+int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
/* May be called with info_r == NULL to check for domain's existance */
int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty
2012-05-23 6:35 [PATCH] [v2] libxl: Add API to retrieve domain console tty Bamvor Jian Zhang
@ 2012-05-29 9:54 ` Ian Campbell
2012-05-29 10:21 ` Ian Jackson
2012-05-29 10:16 ` Ian Jackson
1 sibling, 1 reply; 4+ messages in thread
From: Ian Campbell @ 2012-05-29 9:54 UTC (permalink / raw)
To: Bamvor Jian Zhang; +Cc: jfehlig@suse.com, Ian Jackson, xen-devel@lists.xen.org
On Wed, 2012-05-23 at 07:35 +0100, Bamvor Jian Zhang wrote:
> This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.
>
> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
>
> Changes since v1:
> * Replace function libxl__sprintf with macro GSPRINTF
> * check return value and handle error for libxl__xs_read and libxl__domain_type
> * merge common code for libxl_primary_console_get_tty and
> libxl_primary_console_exec as libxl__primary_console_find
> * Reformat code to be less than 80 characters.
>
> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.c Wed May 23 14:27:57 2012 +0800
> @@ -1188,7 +1188,8 @@ out:
> return rc;
> }
>
> -int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type)
> +int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> + libxl_console_type type)
> {
> GC_INIT(ctx);
> char *p = libxl__sprintf(gc, "%s/xenconsole", libxl__private_bindir_path());
> @@ -1214,24 +1215,74 @@ out:
> return ERROR_FAIL;
> }
>
> -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
> + libxl_console_type type, char **path)
> +{
> + GC_INIT(ctx);
> + char *dom_path = 0;
> + char *tty_path = 0;
> + char *tty = 0;
> + int rc = 0;
> +
> + dom_path = libxl__xs_get_dompath(gc, domid);
> + if (!dom_path) {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + switch (type) {
> + case LIBXL_CONSOLE_TYPE_SERIAL:
> + tty_path = GCSPRINTF("%s/serial/0/tty", dom_path);
> + break;
> + case LIBXL_CONSOLE_TYPE_PV:
> + if (cons_num == 0)
> + tty_path = GCSPRINTF("%s/console/tty", dom_path);
> + else
> + tty_path = GCSPRINTF("%s/device/console/%d/tty", dom_path,
> + cons_num);
> + break;
> + default:
> + rc = ERROR_FAIL;
Strictly this ought to be ERROR_INVAL.
> + goto out;
> + }
> +
> + tty = libxl__xs_read(gc, XBT_NULL, tty_path);
> + if (tty)
> + *path = strdup(tty);
> + else
> + rc = ERROR_FAIL;
> +
> +out:
> + GC_FREE;
> + return rc;
> +}
> +
> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm,
Generally since this is an internal function it should take a libxl__gc
*gc not a ctx and drop the GC_INIT and GC_FREE. You can use the CTX
macro to get a ctx where you need one.
However since the two callers are thinish wrappers around this and you'd
then need the GC_INIT, GC_FREE stuff there I'm inclined to suggest we
make an exception here and leave it as is, Ian J what do you think?
> + uint32_t *domid_out, int *cons_num_out,
> + libxl_console_type *type_out)
> {
> GC_INIT(ctx);
> uint32_t stubdomid = libxl_get_stubdom_id(ctx, domid_vm);
> - int rc;
> - if (stubdomid)
> - rc = libxl_console_exec(ctx, stubdomid,
> - STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV);
> - else {
> + int rc = 0;
> +
> + if (stubdomid) {
> + *domid_out = stubdomid;
> + *cons_num_out = STUBDOM_CONSOLE_SERIAL;
> + *type_out = LIBXL_CONSOLE_TYPE_PV;
> + } else {
> switch (libxl__domain_type(gc, domid_vm)) {
> case LIBXL_DOMAIN_TYPE_HVM:
> - rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_SERIAL);
> + *domid_out = domid_vm;
> + *cons_num_out = 0;
> + *type_out = LIBXL_CONSOLE_TYPE_SERIAL;
> break;
> case LIBXL_DOMAIN_TYPE_PV:
> - rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
> + *domid_out = domid_vm;
> + *cons_num_out = 0;
> + *type_out = LIBXL_CONSOLE_TYPE_PV;
> break;
> case -1:
> - LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
> + LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
> rc = ERROR_FAIL;
> break;
> default:
> @@ -1242,6 +1293,33 @@ int libxl_primary_console_exec(libxl_ctx
> return rc;
> }
>
> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +{
> + uint32_t domid_out;
> + int cons_num_out;
> + libxl_console_type type_out;
These aren't really _out vars here...
There isn't much here which warrants a resend IMHO, if Ian J is happy
with the libxl__primary_console_find interface (as discussed above) I'd
be inclined to take it as is unless you really want to do a respin.
> + int rc;
> +
> + rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out,
> + &type_out);
> + if ( rc ) return rc;
> + return libxl_console_exec(ctx, domid_out, cons_num_out, type_out);
> +}
> +
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
> + char **path)
> +{
> + uint32_t domid_out;
> + int cons_num_out;
> + libxl_console_type type_out;
> + int rc;
> +
> + rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out,
> + &type_out);
> + if ( rc ) return rc;
> + return libxl_console_get_tty(ctx, domid_out, cons_num_out, type_out, path);
> +}
> +
> int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
> {
> GC_INIT(ctx);
> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.h Wed May 23 14:27:57 2012 +0800
> @@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
> * case of HVM guests, and before libxl_run_bootloader in case of PV
> * guests using pygrub. */
> int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
> +/* libxl_console_get_tty retrieves the specified domain's console tty path
> + * and stores it in path. Caller is responsible for freeing the memory.
> + */
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
> + libxl_console_type type, char **path);
> +/* libxl_primary_console_get_tty retrieves the specified domain's primary
> + * console tty path and stores it in path. Caller is responsible for freeing
> + * the memory.
> + */
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
>
> /* May be called with info_r == NULL to check for domain's existance */
> int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty
2012-05-29 9:54 ` Ian Campbell
@ 2012-05-29 10:21 ` Ian Jackson
0 siblings, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2012-05-29 10:21 UTC (permalink / raw)
To: Ian Campbell; +Cc: jfehlig@suse.com, xen-devel@lists.xen.org, Bamvor Jian Zhang
Ian Campbell writes ("Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty"):
> On Wed, 2012-05-23 at 07:35 +0100, Bamvor Jian Zhang wrote:
> > This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.
> >
> > Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
> >
> > Changes since v1:
> > * Replace function libxl__sprintf with macro GSPRINTF
> > * check return value and handle error for libxl__xs_read and libxl__domain_type
> > * merge common code for libxl_primary_console_get_tty and
> > libxl_primary_console_exec as libxl__primary_console_find
> > * Reformat code to be less than 80 characters.
> >
> > diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c Tue May 22 11:55:02 2012 +0100
> > +++ b/tools/libxl/libxl.c Wed May 23 14:27:57 2012 +0800
> > @@ -1188,7 +1188,8 @@ out:
...
> > -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> > +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
> > + libxl_console_type type, char **path)
> > +{
...
> > + switch (type) {
> > + case LIBXL_CONSOLE_TYPE_SERIAL:
...
> > + default:
> > + rc = ERROR_FAIL;
>
> Strictly this ought to be ERROR_INVAL.
Yes.
> > +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm,
>
> Generally since this is an internal function it should take a libxl__gc
> *gc not a ctx and drop the GC_INIT and GC_FREE. You can use the CTX
> macro to get a ctx where you need one.
I think this is fine. In fact in the future we might want to make
this a public function (but not now).
> However since the two callers are thinish wrappers around this and you'd
> then need the GC_INIT, GC_FREE stuff there I'm inclined to suggest we
> make an exception here and leave it as is, Ian J what do you think?
I think there is no rule against internal functions taking ctx. gc is
more conventional but here I think the balance of convenience is in
favour of what Bamvor has done.
Particularly since the outer two functions are so simple.
> There isn't much here which warrants a resend IMHO, if Ian J is happy
> with the libxl__primary_console_find interface (as discussed above) I'd
> be inclined to take it as is unless you really want to do a respin.
I think my comment about the log message ought to be addressed with a
resend.
The nits could have been fixed in-tree at our leisure but if we're
going to have a resend it would be best to address them all.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty
2012-05-23 6:35 [PATCH] [v2] libxl: Add API to retrieve domain console tty Bamvor Jian Zhang
2012-05-29 9:54 ` Ian Campbell
@ 2012-05-29 10:16 ` Ian Jackson
1 sibling, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2012-05-29 10:16 UTC (permalink / raw)
To: Bamvor Jian Zhang; +Cc: jfehlig@suse.com, Ian Campbell, xen-devel@lists.xen.org
Bamvor Jian Zhang writes ("[PATCH] [v2] libxl: Add API to retrieve domain console tty"):
> This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.
It's looking pretty good.
> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.c Wed May 23 14:27:57 2012 +0800
> @@ -1188,7 +1188,8 @@ out:
...
> -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
> + libxl_console_type type, char **path)
> +{
...
> + tty = libxl__xs_read(gc, XBT_NULL, tty_path);
> + if (tty)
> + *path = strdup(tty);
> + else
> + rc = ERROR_FAIL;
Firstly, you need to log something on error here or this function will
fail without logging anything if the console is broken.
Secondly, you should use
libxl__strdup(0, tty)
to get the right error behaviour (if strdup's malloc fails). Ie,
Thirdly, this is a rather odd error handling pattern. I would write
tty = libxl__xs_read(gc, XBT_NULL, tty_path);
if (!tty) {
LOGE(ERROR,"unable to read console tty path `%s'",tty_path);
rc = ERROR_FAIL;
goto out;
}
leaving the main flow to carry on:
*path = libxl__strdup(0, tty);
rc = 0;
Fourthly, you can then leave rc uninitialised at the top of the
function. Any path which as a result gets to the exit with it
uninitialised will produce a compiler warning which would previously
have been erroneously suppressed.
> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm,
> + uint32_t *domid_out, int *cons_num_out,
> + libxl_console_type *type_out)
> {
...
> break;
> case -1:
> - LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
> + LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
Unrelated whitespace change.
> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +{
> + uint32_t domid_out;
> + int cons_num_out;
> + libxl_console_type type_out;
As Ian C says, these shouldn't be called "*_out".
> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.h Wed May 23 14:27:57 2012 +0800
> @@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
> * case of HVM guests, and before libxl_run_bootloader in case of PV
> * guests using pygrub. */
> int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
> +/* libxl_console_get_tty retrieves the specified domain's console tty path
> + * and stores it in path. Caller is responsible for freeing the memory.
> + */
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num,
> + libxl_console_type type, char **path);
> +/* libxl_primary_console_get_tty retrieves the specified domain's primary
> + * console tty path and stores it in path. Caller is responsible for freeing
> + * the memory.
> + */
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
>
> /* May be called with info_r == NULL to check for domain's existance */
> int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,
A formatting nit: I think if we're going to have multi-line comments
associated with functions we should have a blank line either side of
the information about the function to visually associate the comment
with the right function.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-05-29 10:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-23 6:35 [PATCH] [v2] libxl: Add API to retrieve domain console tty Bamvor Jian Zhang
2012-05-29 9:54 ` Ian Campbell
2012-05-29 10:21 ` Ian Jackson
2012-05-29 10:16 ` Ian Jackson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).