Re: [libvirt] [PATCH 1/9] Add helpers for resolving path to resources in build tree
On 04/24/2014 01:21 PM, Nehal J Wani wrote:
> + * virFileFindResourceFull:
> + * @filename: libvirt distributed filename without any path
> + * @prefix: optional string to prepend to filename
> + * @suffix: optional string to append to filename
> + * @builddir: location of the binary in the source tree build tree
> + * @installdir: location of the installed binary
> + * @envname: environment variable used to override all dirs
> + *
> + * A helper which will return a path to @filename within
> + * the current build tree, if the calling binary is being
> + * run from the source tree. Otherwise it will return the
> + * path in the installed location.
> + *
> + * If @envname is none-NULL it will override all other
s/none/non/
> + * directory lookup
> + *
> + * Only use this with @filename files that are part of
> + * the libvirt tree, not 3rd party binaries/files.
> + */
Shouldn't we be mentioning here that its caller's responsibility to
free the returned value?
Wouldn't hurt; although we tend in general to use 'char *' as a return
of something malloc'd (caller frees) vs. 'const char *' as a return of
something that the caller must not free.
> +char *virFileFindResource(const char *filename,
> + const char *builddir,
> + const char *installdir);
Worth ATTRIBUTE_NONNULL for 1, 2, and 3?
> +char *virFileFindResourceFull(const char *filename,
> + const char *prefix,
> + const char *suffix,
> + const char *builddir,
> + const char *installdir,
> + const char *envname);
Worth ATTRIBUTE_NONNULL for 1, 4, and 5?
I really liked the generalization. I have gone through each patch of
this series and have only one question. The recursive grep:
grep "main(int argc" -r ./*
in libvirt's root directory, gives the following files:
./daemon/libvirtd.c ./examples/domsuspend/suspend.c
./examples/hellolibvirt/hellolibvirt.c ./examples/openauth/openauth.c
./examples/object-events/event-test.c ./src/lxc/lxc_controller.c
./src/security/virt-aa-helper.c ./src/locking/lock_daemon.c
./src/locking/sanlock_helper.c ./src/util/iohelper.c
./src/storage/parthelper.c ./tests/test_conf.c ./tests/testutils.h
./tests/testutils.h ./tests/commandhelper.c ./tests/shunloadtest.c
./tests/ssh.c ./tests/seclabeltest.c ./tools/virt-host-validate.c
./tools/virt-login-shell.c ./tools/virsh.c
Shouldn't we be calling virFileActivateDirOverride(argv[0]) in all of them?
We really only need to call it within binaries that are themselves
attempting to locate another resource. This rules out all of examples/
(those should be stand-alone, and not trying to use libvirt internals),
as well as any of the test binaries that don't link against libvirt.so
(commandhelper.c, ssh.c). testutils.h doesn't matter (we only care
about the .c, not the macro magic in the .h). tools/virsh.c is
borderline - we tried to write it as an example program that sticks to
public libvirt interfaces rather than mucking with libvirt internals so
that other clients could copy from it. It's also fairly easy to see
that some others are standalone (iohelper.c and parthelper.c don't call
out to many libvirt functions).
Really, the easiest way to test whether this patch does the right thing,
at least for what currently matters, is to copy libvirt.git into a
virgin machine that lacks any installed libvirt, and prove that 'make
check', './run daemon/libvirtd', and './run tools/virsh' still work;
I'm
in the middle of trying to set up a VM for just that purpose. But I'm
okay with this patch series going in now, even if we missed something
that could also benefit from using this API - we can do followup patches
before 1.2.4 is actually released if we can spot any further problems,
and I'd like to maximize the testing time before we freeze for release.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)