On Fri, Mar 26, 2010 at 10:53:01AM -0600, Eric Blake wrote:
> On 03/26/2010 09:43 AM, Daniel Veillard wrote:
> > + if (stat(path, &sb) < 0) {
> > + ret = 0;
> > + VIR_DEBUG("No hook script %s", path);
> > + } else {
> > + if (access(path, X_OK) != 0) {
>
> Should we also check for !S_ISDIR(&sb.st_mode), so that we explicitly
> reject directories here, rather than failing later when trying to
> execute them? Or go one step further and require regular files, with
> the stricter check for S_ISREG(&sb.st_mode)? (Note: symlinks to regular
> files would still be okay, given that you used stat().)
Right, I'm adding this
> > + * virHookInitialize:
> > + *
> > + * Initialize syncronous hooks support.
>
> s/syncronous/synchronous/
>
> > +/**
> > + * virHookPresent:
> > + * @driver: the driver number (from virHookDriver enum)
> > + *
> > + * Check if a hook exists for the given driver, this is needed
> > + * to avoid unecessary work if the hook is not present
>
> s/unecessary/unnecessary/
>
> > +/*
> > + * virHookCall:
> > + * @driver: the driver number (from virHookDriver enum)
> > + * @id: an id for the object '-' if non available for example on daemon hooks
> > + * @op: the operation on the id e.g. VIR_HOOK_QEMU_OP_START
> > + * @sub_op: a sub_operation, currently unused
> > + * @extra: optional string informations
>
> s/informations/information/ (multiple times)
>
> > + * @input: extra input given to the script on stdin
> > + *
> > + * Implement an Hook call, where the external script for the driver is
>
> s/an Hook/a Hook/
> English is funny on 'a' vs. 'an' before a word starting in 'h';
> sometimes you just have to use a native speaker to get it right ;)
fixed the typos, thanks for raising them, btw I would normally spot
'an hook' as a problem, but I guess I didn't reread thoise comments,
thanks for spotting those !
> > + /*
> > + * Convenience macros borrowed from qemudBuildCommandLine()
> > + */
>
> Duplicating the definition of all these helpers is a bit of a long
> distraction in the middle of this function; is it worth a helper file,
> where we could #include "command_line_builder.h" to get all these
> helpers defined, and to reduce the duplication from
> qemudBuildCommandLine by having the macros in one place?
We can clean this up as Dan suggested, but that should be done
separately, there is a few place in the code where we do this
> > + ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd,
> > + VIR_EXEC_NONE | VIR_EXEC_NONBLOCK);
> > + close(pipefd[1]);
>
> Should we be checking for close() failures, and reporting a system error?
yes, but not changing behaviour, I fixed those allowed me to spot that
if (pipefd[0] > 0)
test on function exit should really be >= as 0 is a valid fd, so fixed
those too
> I only saw minor problems as pointed out above; the overall patch looks
> technically sound.
thanks,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list