Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

On Wed, 04 Jan 2012 13:03:26 -0700
Eric Blake <address@hidden> wrote:

> On 01/04/2012 12:45 PM, Luiz Capitulino wrote:
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +
> > +        setsid();
> > +        fclose(stdin);
> > +        fclose(stdout);
> > +        fclose(stderr);
> > +
> > +        execlp(pmutils_bin, pmutils_bin, NULL);
> 
> It's generally a bad idea to exec a child process without fd 0, 1, and 2
> open on something, even if that something is /dev/null.  POSIX says that
> the system may, but not must, reopen fds on your behalf, and that the
> child without open std descriptors is then executing in a non-conforming
> environment and may misbehave in unexpected manners.

You're right. I just copied what we do in qmp_guest_shutdown()... I think we
have to open /dev/null then, do you agree Michael?

I think I'm doing to use dup2(), like dup2(dev_null_fd, 0). Any better
recommendation?

> 
> > +
> > +        /* 
> > +         * The exec call should not return, if it does something went 
> > wrong.
> > +         * In this case we try to suspend manually if 'mode' is 'hibernate'
> > +         */
> > +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> > +        slog("trying to suspend using the manual method...\n");
> > +
> > +        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> 
> Worse, since you _just_ closed stdin above, fd here will most likely be
> 0, but a O_WRONLY stdin is asking for problems.
> 
> > +        if (fd < 0) {
> > +            slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE,
> > +                    strerror(errno));
> 
> Also, I have no idea where slog() writes to, but since you closed
> stderr, if slog() is trying to use stderr, your error messages would be
> invisible.
>