[Freeciv-Dev] Re: Sound patches to be added (diffs'll come later)

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

Jason Dorje Short <jshort@xxxxxxxxxxxxx> wrote:
> David Weisgerber wrote:
> > Am Samstag, 18. August 2001 10:25 schrieben Sie:
> 
> <snip sound.[ch] files for adding sound support>
> 
> > > I'm also not quite sure what strcoll() is supposed to be doing.  Could
> > > you add a comment on that one?
> > 
> > The strcoll() function compares the two strings s1 and s2.
> >        It returns an integer less than, equal to, or greater than
> >        zero  if  s1  is  found, respectively, to be less than, to
> >        match, or be greater than s2.  The comparison is based  on
> >        strings  interpreted as appropriate for the program's cur­
> >        rent locale for category LC_COLLATE.  (See  setlocale(3)).
> 
> Hmmm.  It seems odd that sound code should be checking translated
> strings.  There should be a comment as to why it's better to do so.
> 
> > > You should probably check the return value of the system() call that
> > > plays the sound.
> > 
> > and what sould I do with the return value ??? That doesn't bring something 
> > as
> > you can see from the discussion. Either play doesn't fail than there is a
> > sound play or play fails and it is like it was until this patch came.
> 
> My man page says that if the return value is 127 (and errno is set
> appropriately) it means the program failed to execute.  

Uh, well, actually it means that the execve() of /bin/sh failed, which
will never happen on a properly configured system.  Basically, the
return value of system() is pretty amazingly useless...

I think a case like this calls for rolling your own execution
function, like the following:


#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>

/* Set the FD_CLOEXEC flag of desc if value is nonzero,
   or clear the flag if value is 0.
   Return 0 on success, or -1 on error with errno set. */

int
set_cloexec_flag (int desc, int value)
{
  int oldflags = fcntl (desc, F_GETFD, 0);
  /* If reading the flags failed, return error indication now.
  if (oldflags < 0)
    return oldflags;
  /* Set just the flag we want to set. */
  if (value != 0)
    oldflags |= FD_CLOEXEC;
  else
    oldflags &= ~FD_CLOEXEC;
  /* Store modified flag word in the descriptor. */
  return fcntl (desc, F_SETFD, oldflags);
}


/* Execute the command specified in argv, return the exit status of
   the child in status.  Return value is 0 if execution of the child
   worked (regardless of its exit status) or the errno value that the
   child got when its exec() failed. */
int run (char *argv[], int *status)
{
    int pid, wpid, err, rv, pipefd[2];
    FILE *pipefile;
    char buf[128], *rbuf;

    if (pipe(pipefd) < 0) {
        return errno;
    }
    pid = fork();
    if (pid < 0)
        return errno;
    if (pid > 0) {
        /* Parent */
        close (pipefd[1]);
        pipefile = fdopen(pipefd[0], "r");
        if (pipefile == NULL) return errno;
        rbuf = fgets(buf, 128, pipefile);
        fclose(pipefile);
        waitpid(pid, status, 0);
        if (rbuf != NULL)
            return WEXITSTATUS(*status);
        return 0;
    } else {
        /* Child */
        close (pipefd[0]);
        set_cloexec_flag(pipefd[1], 1);
        execvp(argv[0], argv);
        err = errno;
        sprintf(buf, "%d", err);
        rv = write(pipefd[1], buf, strlen(buf));
        close(pipefd[1]);
        exit(err);
    }
}



With the following sample usage:


#include <sys/types.h>
#include <sys/wait.h>
#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[])
{
    int rv, status;

    rv = run(&(argv[1]), &status);
    if (rv != 0) {
        printf ("run failed: %s\n", strerror(rv));
        return rv;
    }
    if (WIFEXITED(status))
        return WEXITSTATUS(status);
    else {
        printf("Child didn't exit normally, status = %d...\n", status);
        if (WIFSIGNALED(status))
            printf("Child got signal %d\n", WTERMSIG(status));
        else if (WIFSTOPPED(status))
            printf("Child is stopped with signal %d\n", WSTOPSIG(status));
        else
            printf("Child died for unknown reasons!!  Status = %d\n", status);
    }
    return 1;
}



> An error message (to freelog or possibly even stderr) should be sent
> in this case.  Otherwise sound will silently fail!

Not only should it send an error message, but it should stop trying to
play sounds (of that type if there are multiple sound types
configured) if the failure is that the player couldn't be found.


-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

    It's really hard to define what "unexpected behavior" means when you're
                       talking about Windows.