Re: g_exec_* implementation

[Owen Taylor <otaylor redhat com>]

Havoc Pennington <hp@redhat.com> writes:

> >  gboolean g_exec_sync_with_callback (const gchar         *working_directory,
> >                                      gint                argc,
> >                                      gchar             **argv,
> >                                      GExecFlags          flags,
> >                                      GExecChildSetupFunc child_setup,
> >                                      gpointer            user_data,
> >                                      GExecCallback       callback,
> >                                      GError            **error);
> >
> 
> Seems very valuable to somehow imply in the function name that 
> this will use the main loop.

Well, it's not like it invisibly uses the main loop. That is,
it sets up a callback.
 
> Maybe even put it with the other main loop sources, i.e. 
> make it a function analagous to g_idle_add()?

It could be done that, though it's rather different in that it is 1
shot, and operations like removing it don't make a lot of sense.
  
> Or call it g_exec_nonblocking() maybe, I don't know. (It isn't really
> synchronous is it? so _sync_ doesn't seem right to me.)

Yes, sync is a little funny... maybe just g_exec_with_callback()
  
> >  This shouldn't be too bad to implement on top of g_exec_async().
> >
> 
> I'm not actually sure how to find out when the child exits without
> calling blocking in waitpid() (a custom main loop source that calls
> waitpid() with WNOHANG?). Don't want to use SIGCHLD I don't think.
  
One fairly decent way is just to waitpid() when you get EOF on
stderr and stdout. Works for 90% of the cases, though not always.

If we add something like Tim's posix signal sources, then we
could use SIGCHLD with some more sanity, though it would have
to be clear that mixing use of the function with other handling
of SIGCHLD can't be done with any safety.

Needs some thought.

[...]

> > > #define _(x) x
> > 
> > Hmm, I guess we should go ahead and add gettext support to glib.
> > (With GError, we are producing user visible errors now.)
> > 
> 
> Yep.

Maybe we can scare up a volunteer to do this. It needs to be done
basically like GTK+, though I'd like to see if we can do completely
without the intl/ directory and gettextize. (Avoiding suprises from
make uninstall.)
 
> > >   /* Assuming UTF8 is already validated */
> > 
> > Is the intent here to actually call g_utf8_validate, or do
> > we simply fall over if not?
> >    
> 
> The intent is g_return_if_fail (g_utf8_is_valid (command_line)), but
> that seems too expensive if we aren't going to use G_DISABLE_CHECKS,
> so it isn't there. Not sure what the right thing to do is.
> 
> > >           /* End of an arg, append it to argv,
> > >            */
> > >           
> > >           argv = g_realloc (argv, (argc + 2) * sizeof(gchar*));
> > >           argv[argc] = g_strdup (argstr->str);
> > >           g_string_truncate (argstr, 0);
> > 
> > This results in ' foo bar' giving '' 'foo' 'bar'.
> >
> 
> I don't think it does (this code only gets called outside of quotes,
> right?)

The '' quotes here aren't part of the string. What I mean is,
I think white space at the beginning of the command line will
generate an empty argument.  
  
> > Also, you don't get 'foo ""' right (should be 'foo' '')
> > 
> 
> Ugh, poptParseArgvString() screws this up too. I'm not sure I
> understand what rule results in that happening, so I'll look into it.
> Probably means rewriting the whole function.

I don't mean nested quotes - the results of parsing the string
[foo ""] should be a single argument [foo ""]. (Using [] for
quoting to avoid confusion here.) I'm talking about empty
arguments. (As we were talking about earlier.)
 
> I'll fix the issue with argstr->len > 0 then also ('' should become an
> empty arg if it occurs at the end of a string).

At the end of a string? Also in the middle of a command line
if it is surrounded by whitespace. 

[...]

> > Using waitpid() and expecting to reliably have catch your
> > child exiting, unfortunately is not a completely safe assumption
> > in a library. 
> > 
> > There is not a whole lot you can do if the program is handling
> > SIGCHLD itself, but your probably should at least handle
> > things more gracefully if you get ECHILD. When exit_status
> > is NULL, I think you can treat this as not an error at all.
> > 
> > When exit_status is non-NULL, then I'm not sure if it is better
> > to pretend that it succeeded, pretend it succeeded but
> > put a g_warning ("Child disappeared"), or to return a
> > GError.
> >
> 
> I'm going to say:
> 
>  - pretend that it succeeded doesn't look possible, since there 
>    isn't anything sane to set *exit_status to (that I know of,
>    there wouldn't be a portable value to assign to it)

Well, 0. If you g_warning(), that should be OK.

>  - returning a GError doesn't make sense since if the program 
>    has SIG_IGN on SIGCHLD, they will get the GError 100% of the 
>    time, right? So it's just a programming bug to call exec_sync() 
>    in this case.

I think it is a race condition, it's probably not 100% of the time. 
But it is definitely a programming error.
 
> So I added:
> 
>        else if (errno == ECHILD)
>         {
>           if (exit_status)
>             {
>               g_warning ("In call to g_exec_sync(), exit status of a child process was requested but SIGCHLD action was set to SIG_IGN and ECHILD was received by waitpid(), so exit status can't be returned. This is a bug in the program calling g_exec_sync(); either don't request the exit status, or don't set the SIGCHLD action.");

a) Use 

  "asdf as dfasd fa sdf"
  "asdf asd fa sdf asd "

  Makes editing the source code a lot easier

b) It could happen from a real SIGCHLD handler waiting as well, so
   probably safer to say something like:

    g_warning ("In call to g_exec_sync(), exit status of a child process was requested"
               "but ECHILD was received by waitpid(). This probably means that
               "the SIGCHLD action was set to SIG_IGN or you did a waitpid() with"
               "a negative argument. Either don't request the exit status, or"
               "only wait on children you know about.");
   
  Long, and still probably confusing, but a little more accurate.  

>             }
>           else
>             {
>               /* We don't need the exit status. */
>             }
>         }
> 
> For the call to waitpid() on the intermediate child in the async exec, 
> I just pretend waitpid() succeeded if I get ECHILD.

[...]

> > Actually, I think it is highly possible. Maybe you mean
> > bytes > sizeof(gint)*2.
> >
> 
> If bytes == sizeof(gint)*2 then we should have gotten EOF (chunk == 0)
> and bailed out of the loop already. If we got bytes > sizeof(gint)*2
> then there was unexpected data on the pipe, which most likely means
> the code is broken. Either way we should quit the loop and stop
> reading data.
> 
> This whole check is just paranoia against the code being broken, I
> don't think it should actually be reached...

Look at the code again. It's going to reach bytes == sizeof(gint)*2
_before_ the subsequent read that returns an EOF, unless I'm
missing something.

(And your helper function has sizeof(gint)*2 while it should
have sizeof(gint) * n_bytes_in_buf.)

======
  while (TRUE)
    {
      gint chunk;

      if (bytes >= sizeof(gint)*2)
        break; /* give up, who knows what happened, should not be
                * possible.
                */
    again:
      chunk = read (fd,
                    ((gchar*)buf) + bytes,
                    sizeof(gint)*n_ints_in_buf - bytes);
      if (chunk < 0 && errno == EINTR)
        goto again;
          
      if (chunk < 0)
        {
          /* Some weird shit happened, bail out */
              
          g_set_error (error,
                       G_EXEC_ERROR,
                       G_EXEC_ERROR_FAILED,
                       _("Failed to read from child pipe (%s)"),
                       g_strerror (errno));

          return FALSE;
        }
      else if (chunk == 0)
        break; /* EOF */
      else
        {
          g_assert (chunk > 0);
              
          bytes += chunk;
        }
    }
======

My suggested rewrite is:

======
  while (bytes < sizeof(gint) * n_ints_in_buf)
    {
      gint chunk;

      chunk = read (fd,
                    ((gchar*)buf) + bytes,
                    sizeof(gint)*n_ints_in_buf - bytes);
      if (chunk < 0)
        {
          if (errno != EINTR)
            {
              /* Some weird shit happened, bail out */
              
             g_set_error (error,
                          G_EXEC_ERROR,
                          G_EXEC_ERROR_FAILED,
                          _("Failed to read from child pipe (%s)"),
                          g_strerror (errno));

             return FALSE;
            }
        }
      else if (chunk == 0)
        break; /* EOF */
      else
        bytes += chunk;
    }
======
 
[...]

> So the outstanding issues are:
> 
>  - Think through and implement the main-loop-using version of
>    g_exec_sync()
> 
>  - Reimplement g_parse_argv() to properly handle nested quotes
>    and a couple other issues

Yeah, basically that its. The first probably can wait.
 
                                        Owen