libxl parser question

libxl parser question

[Goncalo Gomes]

[-- Attachment #1: Type: text/plain, Size: 3467 bytes --]

I'm currently working with xen-unstable changeset 25260:0f53540494f7 
and trying to add a `vncviewer` type to the libxl_types.idl, but 
having difficulties understanding the root of a failure that occurs 
after I add my changes. Hopefully I enclose enough detail below, but 
if not, I'm happy to provide more information.

I've created a small patch to add a 'vncviewer' bool type to the 
configuration parser of xl and based these changes on the "localtime" 
implementation part of c/s 25131, see below:

diff -r 0f53540494f7 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c        Fri May 04 11:18:48 2012 +0100
+++ b/tools/libxl/libxl_create.c        Sat May 05 21:27:41 2012 +0000
@@ -156,6 +156,7 @@ int libxl__domain_build_info_setdefault(
     if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT)
         b_info->target_memkb = b_info->max_memkb;
 
+    libxl_defbool_setdefault(&b_info->vncviewer, false);
     libxl_defbool_setdefault(&b_info->localtime, false);
 
     libxl_defbool_setdefault(&b_info->disable_migrate, false);
diff -r 0f53540494f7 tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl       Fri May 04 11:18:48 2012 +0100
+++ b/tools/libxl/libxl_types.idl       Sat May 05 21:27:41 2012 +0000
@@ -250,6 +250,7 @@ libxl_domain_build_info = Struct("domain
     ("video_memkb",     MemKB),
     ("shadow_memkb",    MemKB),
     ("rtc_timeoffset",  uint32),
+    ("vncviewer",       libxl_defbool),
     ("localtime",       libxl_defbool),
     ("disable_migrate", libxl_defbool),
     ("cpuid",           libxl_cpuid_policy_list),
diff -r 0f53540494f7 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c  Fri May 04 11:18:48 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c  Sat May 05 21:27:41 2012 +0000
@@ -728,6 +728,7 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long(config, "rtc_timeoffset", &l, 0))
         b_info->rtc_timeoffset = l;
 
+    xlu_cfg_get_defbool(config, "vncviewer", &b_info->localtime, 0);
     xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);
 
     if (!xlu_cfg_get_long (config, "videoram", &l, 0))

The outcome of this patch when applied and compiled in is that xl 
aborts during the parse_config_data() call in the create_domain().

# ./xl create win7
Parsing config file win7
Aborted

This abort is the `default` case in the switch at xl_cmdimpl.c:736, 
which gets triggered from an erroneous b_info->type with a bogus value 
of 0x84 (which is neither PV nor HVM.)

For reference, the backtrace is:

(gdb) bt
#0  0xb7fe2416 in __kernel_vsyscall ()
#1  0xb7e28781 in *__GI_raise (sig=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0xb7e2bbb2 in *__GI_abort () at abort.c:92
#3  0x0804fe4b in parse_config_data (configfile_filename_report=<value 
optimized out>, configfile_data=<value optimized out>, 
configfile_len=250, d_config=0xbffff55c) at xl_cmdimpl.c:841
#4  0x080526ba in create_domain (dom_info=<value optimized out>) at 
xl_cmdimpl.c:1646
#5  0x0805c098 in main_create (argc=2, argv=0xbffffcc8) at 
xl_cmdimpl.c:3470
#6  0x0804cfe4 in main (argc=3, argv=0xbffffcc4) at xl.c:176

I've also attached the win7 config file which I'm using, in case this 
is a side effect of something wrong in config file itself (I should 
add that it's mostly copied from an example and it successfully 
creates a win7 domain without my patch applied.)

Any guesses or suggestions how to troubleshoot this further would be 
greatly appreciated.

Thanks,
Goncalo


[-- Attachment #2: win7 --]
[-- Type: text/plain, Size: 250 bytes --]

builder = "hvm"
name = "win7"
memory = 1024
acpi = 1
apic = 1
disk = [ 'file:/root/vm/win7/disk1.img,hda,w', 'file:/root/win7.iso,hdc:cdrom,r' ]
device_model = "/usr/lib/xen/bin/qemu-dm"
boot="dca"
kernel = "/usr/lib/xen/boot/hvmloader"
vncviewer=1


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: libxl parser question

[Ian Campbell]

On Sat, 2012-05-05 at 19:19 -0400, Goncalo Gomes wrote:
> diff -r 0f53540494f7 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c  Fri May 04 11:18:48 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c  Sat May 05 21:27:41 2012 +0000
> @@ -728,6 +728,7 @@ static void parse_config_data(const char
>      if (!xlu_cfg_get_long(config, "rtc_timeoffset", &l, 0))
>          b_info->rtc_timeoffset = l;
>  
> +    xlu_cfg_get_defbool(config, "vncviewer", &b_info->localtime, 0);
>      xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);

You have a small typo here (localtime instead of vncviewer), but that
won't effect the crux of this issue.

I've tried reproducing using your config file with the patch applied and
I can't.

> [...]
> This abort is the `default` case in the switch at xl_cmdimpl.c:736, 
> which gets triggered from an erroneous b_info->type with a bogus value 
> of 0x84 (which is neither PV nor HVM.)

I think it might be useful to sprinkle prints of b_info->type everywhere
from the call to libxl_domain_build_info_init_type up until this switch
statement to see if you can identify the line which is overwriting this
field. I couldn't spot it by eye but something in there is presumably
blowing off the end of a buffer or something similar.

You should probably also validate the initial value, which comes from
c_info->type, and if that is wrong trace it back to the place which sets
that initial value.

Ian.

Re: libxl parser question

[Goncalo Gomes]

On Sun, 06 May 2012, Ian Campbell wrote:

> I've tried reproducing using your config file with the patch applied and
> I can't.
> 
> > [...]
> > This abort is the `default` case in the switch at xl_cmdimpl.c:736, 
> > which gets triggered from an erroneous b_info->type with a bogus value 
> > of 0x84 (which is neither PV nor HVM.)
> 
> I think it might be useful to sprinkle prints of b_info->type everywhere
> from the call to libxl_domain_build_info_init_type up until this switch
> statement to see if you can identify the line which is overwriting this
> field. I couldn't spot it by eye but something in there is presumably
> blowing off the end of a buffer or something similar.
> 
> You should probably also validate the initial value, which comes from
> c_info->type, and if that is wrong trace it back to the place which sets
> that initial value.

Running 'make install' after various attempts seems to have done it. 

I was all along under the impression that the dynamic linker would 
pick the libxenlight in the current dir, which is why I hadn't run 
'make install' every so often, but did instead occasionally run 'make 
clean all' Coming to think of it, if the DLD was to pick the library 
from the current directory, a world of security bugs and pain would 
ensue, but for some reason I kept expecting the behaviour to be that.

Thanks though!

Goncalo

Re: libxl parser question

[Ian Campbell]

On Sun, 2012-05-06 at 22:23 +0100, Goncalo Gomes wrote:
> On Sun, 06 May 2012, Ian Campbell wrote:
> 
> > I've tried reproducing using your config file with the patch applied and
> > I can't.
> > 
> > > [...]
> > > This abort is the `default` case in the switch at xl_cmdimpl.c:736, 
> > > which gets triggered from an erroneous b_info->type with a bogus value 
> > > of 0x84 (which is neither PV nor HVM.)
> > 
> > I think it might be useful to sprinkle prints of b_info->type everywhere
> > from the call to libxl_domain_build_info_init_type up until this switch
> > statement to see if you can identify the line which is overwriting this
> > field. I couldn't spot it by eye but something in there is presumably
> > blowing off the end of a buffer or something similar.
> > 
> > You should probably also validate the initial value, which comes from
> > c_info->type, and if that is wrong trace it back to the place which sets
> > that initial value.
> 
> Running 'make install' after various attempts seems to have done it. 
> 
> I was all along under the impression that the dynamic linker would 
> pick the libxenlight in the current dir, which is why I hadn't run 
> 'make install' every so often, but did instead occasionally run 'make 
> clean all' Coming to think of it, if the DLD was to pick the library 
> from the current directory, a world of security bugs and pain would 
> ensue, but for some reason I kept expecting the behaviour to be that.

Yeah, "." is not normally in the default library lookup path for a
variety of security etc issues as you suspected.You can either use
LD_LIBRARY_PATH=stuff:other-stuff or just run make install.

Ian.

Re: libxl parser question

[George Dunlap]

On Tue, May 8, 2012 at 11:55 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> Yeah, "." is not normally in the default library lookup path for a
> variety of security etc issues as you suspected.You can either use
> LD_LIBRARY_PATH=stuff:other-stuff or just run make install.

...or if you're on a .deb-based system, "make deb && dpkg -i
dist/xen-*.deb".  That makes a nice easy way to un-install as well.

 -George