diff -r 9dda0efd8ce1 -r 4a6043e1434a tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Fri Apr 27 17:57:55 2012 +0200
+++ b/tools/libxl/libxl.c Sat Apr 28 22:36:56 2012 +0800
@@ -15,6 +15,8 @@
*/
#include "libxl_osdeps.h"
+//#include "libxl_osdeps.h"
+//#include "libxl_osdeps.h"
#include "libxl_internal.h"
@@ -1173,6 +1175,29 @@ int libxl_primary_console_exec(libxl_ctx
return rc;
}
+int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path)
+{
+ GC_INIT(ctx);
+ char *dom_path = 0;
+ char *tty_path = 0, *os_type_path = 0, *vm_uuid_path = 0;
+ char *tty = 0, *os_type = 0, *vm_uuid = 0;
+
+ dom_path = libxl__xs_get_dompath(gc, domid);
+ vm_uuid_path = libxl__sprintf(gc, "%s/vm", dom_path);
+ vm_uuid = libxl__xs_read(gc, XBT_NULL, vm_uuid_path);
+ os_type_path = libxl__sprintf(gc, "%s/image/ostype", vm_uuid);
+ os_type = libxl__xs_read(gc, XBT_NULL, os_type_path);
+ if ( !strcmp("hvm", os_type)) {
+ tty_path = libxl__sprintf(gc, "%s/serial/0/tty", dom_path);
+ } else {
+ tty_path = libxl__sprintf(gc, "%s/console/tty", dom_path);
+ }
+ tty = libxl__xs_read(gc, XBT_NULL, tty_path);
+ *path = strdup(tty);
+ GC_FREE;
+ return 0;
+}
+
int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
{
GC_INIT(ctx);
diff -r 9dda0efd8ce1 -r 4a6043e1434a tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Fri Apr 27 17:57:55 2012 +0200
+++ b/tools/libxl/libxl.h Sat Apr 28 22:36:56 2012 +0800
@@ -534,6 +534,10 @@ 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_get_console_tty get the tty path from xenstore according to the
+ * domain id.
+ */
+int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, 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,
Bamvor Jian Zhang writes ("[Xen-devel] [PATCH] [mq]:
patch_libxl_get_console_tty.diff"):> diff -r 9dda0efd8ce1 -r 4a6043e1434a tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Fri Apr 27 17:57:55 2012 +0200
> +++ b/tools/libxl/libxl.c Sat Apr 28 22:36:56 2012 +0800
> @@ -15,6 +15,8 @@
> */
>
> #include "libxl_osdeps.h"
> +//#include "libxl_osdeps.h"
> +//#include "libxl_osdeps.h"
Thanks for posting this but was int actually inteded for inclusion in
the tree ? If so we need a commit message and a Signed-off-by and so
forth. Also we may be too late for 4.2 considering the feature
freeze.
> +int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path)
> +{
We already have various functions to refer to and open consoles, which
have much of this functionality in them already. That shouldn''t be
duplicated.
Also we need to make a policy decision about whether the fact that the
console looks like a tty is a part of the API.
> +/* libxl_get_console_tty get the tty path from xenstore according to the
> + * domain id.
> + */
> +int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path);
In any case the doc comment should not mention xenstore. It should
also document the memory allocation behaviour.
Ian.
Bamvor Jian Zhang
2012-May-15 05:06 UTC
Re: [PATCH] [mq]: patch_libxl_get_console_tty.diff
Hi, Ian thanks your reply. Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:>Bamvor Jian Zhang writes ("[Xen-devel] [PATCH] [mq]: patch_libxl_get_console_tty.diff"): >> diff -r 9dda0efd8ce1 -r 4a6043e1434a tools/libxl/libxl.c>Thanks for posting this but was int actually inteded for inclusion in >the tree ? If so we need a commit message and a Signed-off-by and so >forth. Also we may be too late for 4.2 considering the feature >freeze.I am sorry maybe there are something wrong in my hgrc settings. the patch including comments and Signed-off-by in my ".hg/patches" directory. I understand that 4.2 is almost release. meanwhile, my patch is useful for adding a api in libvirt for xenlight open console commands.>> +int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path) >> +{ > >We already have various functions to refer to and open consoles, which >have much of this functionality in them already. That shouldn''t be >duplicated. > >Also we need to make a policy decision about whether the fact that the >console looks like a tty is a part of the API.sorry i only found one api about open console is libxl_console_exec. libxl_console_exec call xenconsole command directly which is not compatible with libvirt open console api. libvirt open console want a console device path and handle the console by libvirt itself. libvirt xen(not xenlight) driver read console path from xenstore directly which is prohibited by libvirt xenlight drvier. So, because these reasons, i guess add this simple api to return console path to libvirt is a good choice. it is useful for libvirt and do not break libxl api.> >> +/* libxl_get_console_tty get the tty path from xenstore according to the >> + * domain id. >> + */ >> +int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path); > >In any case the doc comment should not mention xenstore. It should >also document the memory allocation behaviour.I will change it in my next version.> >Ian.Bamvor
On Tue, 2012-05-15 at 06:06 +0100, Bamvor Jian Zhang wrote:> >> +int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path) > >> +{ > > > >We already have various functions to refer to and open consoles, which > >have much of this functionality in them already. That shouldn''t be > >duplicated. > > > >Also we need to make a policy decision about whether the fact that the > >console looks like a tty is a part of the API.> sorry i only found one api about open console is libxl_console_exec. > libxl_console_exec call xenconsole command directly which is not > compatible with libvirt open console api. libvirt open console want a > console device path and handle the console by libvirt itself. libvirt > xen(not xenlight) driver read console path from xenstore directly > which is prohibited by libvirt xenlight drvier. > So, because these reasons, i guess add this simple api to return > console path to libvirt is a good choice. it is useful for libvirt and > do not break libxl api.We actually discussed the existing interface in the context of the libxl stable API (sub-thread starting at <20357.44324.27939.514126@mariner.uk.xensource.com> which has a lot of sub-threads. Another interesting bit starts at <20358.47143.994639.76453@mariner.uk.xensource.com>) In that thread IanJ said:> ] int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass); > ] int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type); > ] /* libxl_primary_console_exec finds the domid and console number > ] * corresponding to the primary console of the given vm, then calls > ] * libxl_console_exec with the right arguments (domid might be different > ] * if the guest is using stubdoms). > ] * This function can be called after creating the device model, in > ] * 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); > > These functions should not exec things for you; they should tell you > the parameters. Any execing helpers should be in libxlu.and later on he said, in response to some of my musings:> But what if your vnc viewer doesn''t have the command line options > these functions expect ? libxl_vncviewer_* should give you an idl > struct containing the ip address (or perhaps the domain name), port > number, and password.I think the same can be said of libxl_console_*. In the end I decided:> this seems like 4.3 material at this stage. > > I''d expect that the functions which behaved this way would not be > called ..._exec (perhaps ..._get_params or so) so implementing the > proper interface in 4.3 won''t cause a compat issue.I think I''d be happy to make a freeze exception for a patch which implemented the returning of an IDL struct representing the console device for the benefit of libvirt, so long as it is pretty much self contained (which I think it will be). I don''t see any need for it to be a requirement to switch xl over to this new interface at this stage, but we could mark the exec functions as already deprecated in 4.2 and plan to do so (with associated libxlu helpers) in 4.3. This still leaves us having to decide if we want to expose the fact that the console is a tty. Perhaps a compromise would be to include a libxl_console_type enum and KeyedUnion of params, currently the only possible value for the enum would be "PTY" (or "TTY")? (or maybe we can leave that until the second one comes along...) Ian.
Bamvor Jian Zhang
2012-May-18 07:28 UTC
Re: [PATCH] [mq]: patch_libxl_get_console_tty.diff
Hi, Ian >>>Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Tue, 2012-05-15 at 06:06 +0100, Bamvor Jian Zhang wrote: > > >> +int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path) > > >> +{ > > > > > >We already have various functions to refer to and open consoles, which > > >have much of this functionality in them already. That shouldn''t be > > >duplicated. > > > > > >Also we need to make a policy decision about whether the fact that the > > >console looks like a tty is a part of the API. > > > sorry i only found one api about open console is libxl_console_exec. > > libxl_console_exec call xenconsole command directly which is not > > compatible with libvirt open console api. libvirt open console want a > > console device path and handle the console by libvirt itself. libvirt > > xen(not xenlight) driver read console path from xenstore directly > > which is prohibited by libvirt xenlight drvier. > > So, because these reasons, i guess add this simple api to return > > console path to libvirt is a good choice. it is useful for libvirt and > > do not break libxl api. > > We actually discussed the existing interface in the context of the libxl > stable API (sub-thread starting at > <20357.44324.27939.514126@mariner.uk.xensource.com> which has a lot of > sub-threads. Another interesting bit starts at > <20358.47143.994639.76453@mariner.uk.xensource.com>) > > In that thread IanJ said: > > ] int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass); > > ] int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, > libxl_console_type type); > > ] /* libxl_primary_console_exec finds the domid and console number > > ] * corresponding to the primary console of the given vm, then calls > > ] * libxl_console_exec with the right arguments (domid might be > different > > ] * if the guest is using stubdoms). > > ] * This function can be called after creating the device model, in > > ] * 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); > > > > These functions should not exec things for you; they should tell you > > the parameters. Any execing helpers should be in libxlu. > > and later on he said, in response to some of my musings: > > But what if your vnc viewer doesn''t have the command line options > > these functions expect ? libxl_vncviewer_* should give you an idl > > struct containing the ip address (or perhaps the domain name), port > > number, and password. > > I think the same can be said of libxl_console_*. > > In the end I decided: > > this seems like 4.3 material at this stage. > > > > I''d expect that the functions which behaved this way would not be > > called ..._exec (perhaps ..._get_params or so) so implementing the > > proper interface in 4.3 won''t cause a compat issue. > > I think I''d be happy to make a freeze exception for a patch which > implemented the returning of an IDL struct representing the console > device for the benefit of libvirt, so long as it is pretty much self > contained (which I think it will be).thanks. I am working on it. i will send the patch v2 soon.> I don''t see any need for it to be a requirement to switch xl over to > this new interface at this stage, but we could mark the exec functions > as already deprecated in 4.2 and plan to do so (with associated libxlu > helpers) in 4.3. > > This still leaves us having to decide if we want to expose the fact that > the console is a tty. Perhaps a compromise would be to include a > libxl_console_type enum and KeyedUnion of params, currently the only > possible value for the enum would be "PTY" (or "TTY")? (or maybe we can > leave that until the second one comes along...) > > Ian. >Bamvor
Bamvor Jian Zhang writes ("Re: [Xen-devel] [PATCH] [mq]:
patch_libxl_get_console_tty.diff"):> Hi, Ian
> > I think I''d be happy to make a freeze exception for a patch
which
> > implemented the returning of an IDL struct representing the console
> > device for the benefit of libvirt, so long as it is pretty much self
> > contained (which I think it will be).
>
> thanks. I am working on it. i will send the patch v2 soon.
Sorry to throw a spanner in the works, but I think actually that this
isn''t really the right approach.
Having the considered the question I think we should indeed expose the
fact that there is (or can be) a tty as part of the libxl API. And I
don''t really want to introduce a new complex IDL struct with only one
user.
So your old interface, along these lines, is good:
int libxl_get_console_tty(libxl_ctx *ctx, uint32_t domid, char **path)
However, it should probably be in teh same pattern as
libxl_console_exec and libxl_primary_console_exec.
So:
int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid,
int cons_num, libxl_console_type type,
char **path);
int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid,
char **path);
These should probably reuse the same innards as the _exec functions.
Are you planning to do anything about libxl_vncviewer_exec ?
Ian.