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

Re: [libvirt] [PATCH 1/7] tests: Add qemuxml2xml tests for <seclabel> handling

On 01/12/2011 10:22 AM, Cole Robinson wrote:
> +++ b/tests/domainschematest
> @@ -4,7 +4,7 @@
>  . $srcdir/test-lib.sh
>  . $abs_srcdir/schematestutils.sh
> -DIRS="domainschemadata qemuxml2argvdata sexpr2xmldata xmconfigdata xml2sexprdata qemuxml2xmloutdata"
> +DIRS="domainschemadata qemuxml2argvdata sexpr2xmldata xmconfigdata xml2sexprdata qemuxml2xmldata"

At first, I though this should be listing _both_ qemuxml2xml{out,}data,
rather than just one or the other, but now I see that you've moved the
only five files that were in that directory so that all qemu*.xml files
are in a single location, and adjusted the test to match.  Nice cleanup,
but incomplete - you also need to tweak tests/Makefile.am to drop
reference to the deleted qemuxml2xmloutdata directory, so that 'make
dist' won't complain.

This would have been a little more obvious to me if you had used git
diff rename compression (git config diff.renames true), rather than
showing the moved files as deletions and creations.

In fact, separating into two patches (file movement consolidation, vs.
new tests), might be nice, if you're up to the task, but I won't insist.

> @@ -67,10 +72,8 @@ static int testCompareXMLToXMLHelper(const void *data) {
>      char xml_out[PATH_MAX];
>      int ret;
> -    snprintf(xml_in, PATH_MAX, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
> -             abs_srcdir, info->name);
> -    snprintf(xml_out, PATH_MAX, "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml",
> -             abs_srcdir, info->name);
> +    snprintf(xml_in, PATH_MAX, input_folder_fmt, abs_srcdir, info->name);
> +    snprintf(xml_out, PATH_MAX, XML2XMLOUT_FMT, abs_srcdir, info->name);

Hmm, this silently ignores overflow if run in a deeply nested hierarchy.
 Someday, we should switch these to virAsprintf (and as a bonus, it
avoids PATH_MAX stack allocation).  But that's independent of this patch.

Conditional ACK, once you fix the tests/Makefile.am EXTRA_DIST nit.

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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