Re: [Xen-devel] [libvirt] [PATCH V4 1/3] Introduce support for parsing/formatting Xen xl config format
Eric Blake wrote: > On 01/13/2015 04:47 PM, Jim Fehlig wrote: > > +++ b/src/Makefile.am @@ -1005,6 +1005,10 @@ XENCONFIG_SOURCES = \ xenconfig/xen_common.c xenconfig/xen_common.h \ xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \ xenconfig/xen_xm.c xenconfig/xen_xm.h +if WITH_LIBXL +XENCONFIG_SOURCES += \ + xenconfig/xen_xl.c xenconfig/xen_xl.h +endif WITH_LIBXL >>> Missing an EXTRA_DIST listing to ensure these two files are part of a >>> tarball even when configure does not build libxl sources (that is, make >>> sure 'make distcheck' will not fail if configured on a machine without >>> libxl support). >>> >>> >> Several of the tests fail on the old distro I'm testing on, which causes >> 'make distcheck' to fail. But I assumed I could simulate it on a newer >> distro with 'configure --without-libxl', yet 'make distcheck' succeeds >> and the files are included in the tarball. Are you seeing a failure on >> RHEL5? >> > > I haven't yet tested a 'make distcheck' on that particular VM of mine; > I'll fire one off and see what happens. It might be simpler to just to > check that if 'make dist' is run when configured --without-libxl, then > it still includes both files in the tarball. > I've verified src/xenconfig/xen_xl.[ch] are included in the tarball when configured --without-libxl. I've sent a V5 which addresses all of your V4 comments https://www.redhat.com/archives/libvir-list/2015-January/msg00494.html Regards, Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] [PATCH V4 1/3] Introduce support for parsing/formatting Xen xl config format
On 01/13/2015 04:47 PM, Jim Fehlig wrote: >>> +++ b/src/Makefile.am >>> @@ -1005,6 +1005,10 @@ XENCONFIG_SOURCES = >>> \ >>> xenconfig/xen_common.c xenconfig/xen_common.h \ >>> xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \ >>> xenconfig/xen_xm.c xenconfig/xen_xm.h >>> +if WITH_LIBXL >>> +XENCONFIG_SOURCES += \ >>> + xenconfig/xen_xl.c xenconfig/xen_xl.h >>> +endif WITH_LIBXL >>> >> >> Missing an EXTRA_DIST listing to ensure these two files are part of a >> tarball even when configure does not build libxl sources (that is, make >> sure 'make distcheck' will not fail if configured on a machine without >> libxl support). >> > > Several of the tests fail on the old distro I'm testing on, which causes > 'make distcheck' to fail. But I assumed I could simulate it on a newer > distro with 'configure --without-libxl', yet 'make distcheck' succeeds > and the files are included in the tarball. Are you seeing a failure on > RHEL5? I haven't yet tested a 'make distcheck' on that particular VM of mine; I'll fire one off and see what happens. It might be simpler to just to check that if 'make dist' is run when configured --without-libxl, then it still includes both files in the tarball. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] [PATCH V4 1/3] Introduce support for parsing/formatting Xen xl config format
Eric Blake wrote: > On 01/13/2015 08:53 AM, Jim Fehlig wrote: > >> Introduce a parser/formatter for the xl config format. Since the >> deprecation of xm/xend, the VM config file format has diverged as >> new features are added to libxl. This patch adds support for parsing >> and formating the xl config format. It supports the existing xm config >> format, plus adds support for spice graphics and xl disk config syntax. >> >> > > >> xl disk config is parsed with the help of xlu_disk_parse() from >> libxlutil, libxl's utility library. Although the library exists >> in all Xen versions supported by the libxl virt driver, only >> recently has the corresponding header file been included. A check >> for the header is done in configure.ac. If not found, xlu_disk_parse() >> is declared externally. >> >> Signed-off-by: Kiarie Kahurani >> Signed-off-by: Jim Fehlig >> --- >> +++ b/src/Makefile.am >> @@ -1005,6 +1005,10 @@ XENCONFIG_SOURCES = >> \ >> xenconfig/xen_common.c xenconfig/xen_common.h \ >> xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \ >> xenconfig/xen_xm.c xenconfig/xen_xm.h >> +if WITH_LIBXL >> +XENCONFIG_SOURCES +=\ >> +xenconfig/xen_xl.c xenconfig/xen_xl.h >> +endif WITH_LIBXL >> > > Missing an EXTRA_DIST listing to ensure these two files are part of a > tarball even when configure does not build libxl sources (that is, make > sure 'make distcheck' will not fail if configured on a machine without > libxl support). > Several of the tests fail on the old distro I'm testing on, which causes 'make distcheck' to fail. But I assumed I could simulate it on a newer distro with 'configure --without-libxl', yet 'make distcheck' succeeds and the files are included in the tarball. Are you seeing a failure on RHEL5? Regards, Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] [PATCH V4 1/3] Introduce support for parsing/formatting Xen xl config format
On 01/13/2015 08:53 AM, Jim Fehlig wrote:
> Introduce a parser/formatter for the xl config format. Since the
> deprecation of xm/xend, the VM config file format has diverged as
> new features are added to libxl. This patch adds support for parsing
> and formating the xl config format. It supports the existing xm config
> format, plus adds support for spice graphics and xl disk config syntax.
>
> xl disk config is parsed with the help of xlu_disk_parse() from
> libxlutil, libxl's utility library. Although the library exists
> in all Xen versions supported by the libxl virt driver, only
> recently has the corresponding header file been included. A check
> for the header is done in configure.ac. If not found, xlu_disk_parse()
> is declared externally.
>
> Signed-off-by: Kiarie Kahurani
> Signed-off-by: Jim Fehlig
> ---
> +++ b/src/Makefile.am
> @@ -1005,6 +1005,10 @@ XENCONFIG_SOURCES =
> \
> xenconfig/xen_common.c xenconfig/xen_common.h \
> xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \
> xenconfig/xen_xm.c xenconfig/xen_xm.h
> +if WITH_LIBXL
> +XENCONFIG_SOURCES += \
> + xenconfig/xen_xl.c xenconfig/xen_xl.h
> +endif WITH_LIBXL
Missing an EXTRA_DIST listing to ensure these two files are part of a
tarball even when configure does not build libxl sources (that is, make
sure 'make distcheck' will not fail if configured on a machine without
libxl support).
> +xenParseXLDisk(virConfPtr conf, virDomainDefPtr def)
> +while (list) {
> +const char *disk_spec = list->str;
> +
> +if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
Over-parenthesized.
> +goto skipdisk;
> +
> +libxl_device_disk_init(libxldisk);
> +
> +if (xlu_disk_parse(xluconf, 1, &disk_spec, libxldisk))
> +goto fail;
> +
> +if (!(disk = virDomainDiskDefNew()))
> +goto fail;
> +
> +if (VIR_STRDUP(disk->dst, libxldisk->vdev) < 0)
> +goto fail;
> +
> +if (virDomainDiskSetSource(disk, libxldisk->pdev_path) < 0)
> +goto fail;
> +
> +disk->src->readonly = libxldisk->readwrite ? 0 : 1;
Isn't disk->src->readonly a bool? In which case, this should be:
disk->src->readonly = !libxldisk->readwrite;
for correct typing.
I'd wait for John to confirm that Coverity is happy, but ACK if you fix
the spots I pointed out.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
