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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list