[libvirt] [PATCH] [3/6] Add hook utilities

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/