[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] allow to set path to xen userspace tools



On Tue, Jul 07, 2009 at 06:02:20PM +0100, Daniel P. Berrange wrote:
> On Tue, Jul 07, 2009 at 01:16:02PM +0200, Guido G?nther wrote:
> > Hi,
> > attached patch makes the path to the xen userspace tools configurable.
> > Debian keeps this under /usr/lib/xen-default/ instead of /usr/lib/xen/.
> > We don't have the amd64 libs in /usr/lib64/xen either so we can use:
> >  ./configure --with-xen-tools=/usr/lib/xen-defaults --with-xen-tools64=/usr/lib/xen-defaults
> > instead of patching src/xen_internals.c directly.
> > Skipping above options gives the current behaviour. I checked that "make
> > check" still passes. O.k. to apply?
> 
> The code changes look ok, but I'm not much of a fan of the way the
> test cases are handled here, with all these data files listed in
> configure.in  
> 
> I think i'd be more inclined to let virtTestLoadFile() do the subsitution
> at time its loading the files. Perhaps
> 
>    if (virtTestLoadFile(xml, &expectxml, MAX_FILE, {
>          "@XEN_TOOLS@", XEN_TOOLS,
>        }) < 0)
>       goto fail;

We'd need to do arbitrary number of string replacements:

    if (virtTestLoadFile(xml, &expectxml, MAX_FILE, {
          "@XEN_TOOLS@", XEN_TOOLS,
          "@XEN_TOOLS64@", XEN_TOOLS64,
          NULL,
        }) < 0)
       goto fail;

into the fixed size buffer expectxml. Doing this via autofoo or via a
makefile rule instead of in C looks more elegant to me.
Cheers,
 -- Guido


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]