Re: [Xen-devel] [PATCH v1 01/02] HVM firmware passthrough libxl support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-01-21 at 17:05 +0000, Ross Philipson wrote:

> I will change that and look at using GCSPRINTF. I sort of just copied the
> logging from elsewhere in that code.

The GC* things are pretty new, likewise the shortened LOG macros.


> > 
> > > +err:
> > > +    if (args->smbios_module.data) {
> > > +        free(args->smbios_module.data);
> > > +        args->smbios_module.data = NULL;
> > > +    }
> > > +    if (args->acpi_module.data) {
> > > +        free(args->acpi_module.data);
> > > +        args->acpi_module.data = NULL;
> > > +    }
> > 
> > DO you leak args->image_file_name here?
> 
> The previous code did not clean up the values returned from
> libxl__abs_path() so I assumed it was like strings returned
> from libxl__sprintf(), ie cleaned up by the GC.

You are right, I missed that this was a gc allocation.

You could add the module data to the gc too btw and have it take care of
everything in that struct, arguably it is less confusing of each struct
only contains one "kind" of pointer. (the other option is to pass NOGC
to libxl__abs_path and manage that by hand too).

> 
> > 
> > > +out:
> > > +    return rc;
> > >  }
> > >
> > >  int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> > 
> > > diff -r 35a0556a7f76 tools/libxl/libxl_types.idl
> > > --- a/tools/libxl/libxl_types.idl       Thu Jan 10 17:32:10 2013 +0000
> > > +++ b/tools/libxl/libxl_types.idl       Fri Jan 18 15:21:50 2013 -0500
> > > @@ -308,6 +308,8 @@ libxl_domain_build_info = Struct("domain
> > >                                         ("vpt_align",
> > libxl_defbool),
> > >                                         ("timer_mode",
> > libxl_timer_mode),
> > >                                         ("nested_hvm",
> > libxl_defbool),
> > > +                                       ("smbios_firmware",  string),
> > > +                                       ("acpi_firmware",    string),
> > >                                         ("nographic",
> > libxl_defbool),
> > >                                         ("vga",
> > libxl_vga_interface_info),
> > >                                         ("vnc",
> > libxl_vnc_info),
> > 
> > I think we ought to have a LIBXL_HAVE_FOO define for these two, in
> > libxl.h I suppose since the IDL doesn't support that sort of thing (not
> > sure if it should or not).
> 
> I think I understand what you are suggesting (having read the comment
> in libxl.h). I guess the thing that really breaks backward compat is
> the code in libxl_dom.c that calls libxc. Should I introduce a
> LIBXL_HAVE_FIRMWARE_PASSTHROUGH and have an additional block in
> Libxl_dom.c with the old code that calls xc_hvm_build_target_mem()?

I don't think that is necessary, all which is required is some way for
users of libxl (like libvirt, or xl if it weren't in tree) to tell if
they can try and use the new *_firmware fields or not. There's no need
to vary the libxl implementation based on it.

> I guess this would be the first instance of a LIBXL_HAVE_* from the
> looks of it.

Right.

> 
> > 
> > > diff -r 35a0556a7f76 tools/libxl/xl_cmdimpl.c
> > > --- a/tools/libxl/xl_cmdimpl.c  Thu Jan 10 17:32:10 2013 +0000
> > > +++ b/tools/libxl/xl_cmdimpl.c  Fri Jan 18 15:21:50 2013 -0500
> > > @@ -882,6 +882,11 @@ static void parse_config_data(const char
> > >          }
> > >
> > >          xlu_cfg_get_defbool(config, "nestedhvm", &b_info-
> > >u.hvm.nested_hvm, 0);
> > > +
> > > +        xlu_cfg_replace_string (config, "smbios_firmware",
> > > +                                &b_info->u.hvm.smbios_firmware, 0);
> > > +        xlu_cfg_replace_string (config, "acpi_firmware",
> > > +                                &b_info->u.hvm.acpi_firmware, 0);
> > >          break;
> > >      case LIBXL_DOMAIN_TYPE_PV:
> > >      {
> > >
> > >
> 



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