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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list