Mailing List Archive: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl

[PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec

Dec 13, 2011, 1:26 AM

Post #1 of 4 (105 views)

# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# Date 1323766195 -3600
# Node ID 573a246bf0c6b3ad01473d350a2c3c5aad30c351
# Parent e3907ed912201e5270ff19265fab2c979b3929cc
libxl: add libxl__forkexec function to libxl_exec

Add a new function to libxl_exec that performs a fork and executes
the passed arguments using libxl__exec.

Signed-off-by: Roger Pau Monne <roger.pau@entel.upc.edu>

diff -r e3907ed91220 -r 573a246bf0c6 tools/libxl/libxl_exec.c
--- a/tools/libxl/libxl_exec.c Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl_exec.c Tue Dec 13 09:49:55 2011 +0100
@@ -450,6 +450,39 @@ int libxl__spawn_check(libxl__gc *gc, li
return ERROR_FAIL;
}

+int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+ int stderrfd, char **args)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ int status;
+ int rc = 0;
+ pid_t pid = fork();
+
+ switch (pid) {
+ case -1:
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fork failed\n");
+ rc = -1;
+ break;
+ case 0:
+ libxl__exec(stdinfd, stdoutfd, stderrfd, args[0], args);
+ /* libxl__exec never returns */
+ default:
+ while (waitpid(pid, &status, 0) < 0) {
+ if (errno != EINTR) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "waitpid failed\n");
+ rc = -1;
+ break;
+ }
+ }
+ if (status)
+ libxl_report_child_exitstatus(ctx, LIBXL__LOG_ERROR,
+ args[0], pid, status);
+ rc = status;
+ break;
+ }
+ return rc;
+}
+
/*
* Local variables:
* mode: C
diff -r e3907ed91220 -r 573a246bf0c6 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Fri Sep 30 14:38:55 2011 +0200
+++ b/tools/libxl/libxl_internal.h Tue Dec 13 09:49:55 2011 +0100
@@ -400,6 +400,22 @@ _hidden int libxl__spawn_check(libxl__gc
_hidden void libxl__exec(int stdinfd, int stdoutfd, int stderrfd,
const char *arg0, char **args); // logs errors, never returns

+/*
+ * libxl__forkexec - Executes a file synchronously
+ * 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
+ *
+ * Returns the exit status, as reported by waitpid.
+ */
+_hidden int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+ int stderrfd, char **args);
+
/* from xl_create */
_hidden int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info, uint32_t *domid);
_hidden int libxl__domain_build(libxl__gc *gc,

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

Re: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec [ In reply to ]

Ian.Jackson at eu

Dec 13, 2011, 7:14 AM

Post #2 of 4 (98 views)

Permalink

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@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec [ In reply to ]

Ian.Campbell at citrix

Dec 13, 2011, 7:45 AM

Post #3 of 4 (97 views)

Permalink

On Tue, 2011-12-13 at 15:14 +0000, Ian Jackson wrote:
>
> > + * 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.)

The definition of args[0] as the actual executable (or argv0) and the
requirement for args[n] == NULL are interesting enough to mention I
think (the NULL thing in particular often trips people up). But you
could also just reference execvp(3).

Ian.

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

Re: [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec [ In reply to ]

Ian.Jackson at eu

Dec 13, 2011, 7:48 AM

Post #4 of 4 (103 views)

Permalink

Ian Campbell writes ("Re: [Xen-devel] [PATCH 03 of 14 v4] libxl: add libxl__forkexec function to libxl_exec"):
> On Tue, 2011-12-13 at 15:14 +0000, Ian Jackson wrote:
> > 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.)
>
> The definition of args[0] as the actual executable (or argv0) and the
> requirement for args[n] == NULL are interesting enough to mention I
> think (the NULL thing in particular often trips people up). But you
> could also just reference execvp(3).

I think it would be better to reference execvp.

Ian.

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