Re: [Xen-devel] [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[Xen-devel] [PATCH 03 of 14 v4] libxl: add 
libxl__forkexec function to libxl_exec"):
> +int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
> +                    int stderrfd, char **args)
> +{
...
> +        libxl__exec(stdinfd, stdoutfd, stderrfd, args[0], args);
...
> +            libxl_report_child_exitstatus(ctx, LIBXL__LOG_ERROR,
> +                                          args[0], pid, status);

I think this function should allow passing separate values for arg0
and args.  It would also be useful to allow passing a separate "what"
to libxl_report_child_exitstatus: for example, that would allow you to
distinguish the various call sites for running a hotplug script, so
that failures in the plug and unplug give different answers.  It also
allows the caller to use libxl__sprintf if they want to give
more information about the program.

> +/*
> + * libxl__forkexec - Executes a file synchronously

OK, but:

> + * gc: allocation pool
> + * stdinfd, stdoutfd, stderrfd: fds to pass to libxl__exec
> + * args: file to execute and arguments to pass in the following format
> + *      args[0]: file to execute
> + *      args[1]: first argument to pass to the called program
> + *      ...
> + *      args[n-1]: (n-1)th argument to pass to the called program
> + *      args[n]: NULL

IMO all of the above is obvious and should be eliminated.  (This is
exactly the kind of "fd is the file descriptor" stuff that I was
complaining about in another recent thread.)

> + * Returns the exit status, as reported by waitpid.

And this fails to go into enough detail.  Indeed it is false.
In particular, it fails to mention:
  - what the function does if some system call fails
  - whether any messages are logged

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel