On Tue, Aug 19, 2008 at 12:44:16PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> > There are several system calls in the virExec function for which we don't
> > or can't report errors. This patch does a couple of things to improve the
> > situation. It moves the code for setting non-block/close-exec to before the
> > fork() so we can report errors for it. It removes the 'dom' and 'net' params
> > from the ReportError function since we deprecated those long ago and all
> > callers simply pass in NULL. It resets the 'virErrorHandler' callback to
> > NULL in the child, so that errors raised will get reported to stderr
> > instead of invoking a callback which is likely no longer valid in the child
> > process. It reports failures to exec the binary and dup file descriptors.
> > All errors in the child will end up on stderr, so they will at least be
> > visible on the parent's console, or a logfile if one was setup for the
> > child. Previously there would just be silent failure.
>
> Looks fine.
> ACK
>
> one "would be nice" comment:
>
> > diff -r 4f44b07c47c1 src/util.c
> ...
> > if (status == NULL) {
> > errno = EINVAL;
> > - return (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0) ? 0 : -1;
> > + if (WIFEXITED(exitstatus) && WEXITSTATUS(exitstatus) == 0)
> > + return 0;
> > +
> > + ReportError(conn, VIR_ERR_INTERNAL_ERROR,
> > + _("%s exited with non-zero status %s"),
>
> Since this code is going to be used in so many contexts, now,
> it'd be nice to report which signal (if any) and the precise
> exit status value.
Yes, good idea - i'll add that when i commit.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list