diff --git a/Makefile.am b/Makefile.am index df1cc90c..cf899d56 100644 --- a/Makefile.am +++ b/Makefile.am @@ -21,4 +21,5 @@ EXTRA_DIST = \ xmms2-plugin/mpg123/mpg123.c \ xmms2-plugin/mpg123/wscript \ makedll.sh \ + test/forkfaint.c \ test/rms16.c diff --git a/NEWS b/NEWS index 24df61a2..478c45b1 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,7 @@ This sort of qualifies as bugfix, actually, since it's item 1786430 on the tracker;-) - Make flexible rate computations more safe (especially on 32bit platforms) by switching to the looping code instead of direct multiplication/division (which blows up with integers). A possible effect was premature track ending with a forced rate. - Flexible rate mode works _at_all_ again with fixed output support detection code (comparison of -1 with a size_t ended a loop all too early). +- check for error in flushing output (this ends mpg123 for a broken pipe, for example) 1.0rc1 ---- diff --git a/src/audio.c b/src/audio.c index ecdf637a..33bc2c55 100644 --- a/src/audio.c +++ b/src/audio.c @@ -378,14 +378,11 @@ int init_output(audio_output_t **ao) audio_output_t *bao = NULL; /* To be clear: That's the buffer's pointer. */ param.usebuffer = 0; /* The buffer doesn't use the buffer. */ /* Open audio output module */ - if(param.outmode == DECODE_AUDIO) + bao = open_output_module(param.output_module); + if(!bao) { - bao = open_output_module(param.output_module); - if(!bao) - { - error("Failed to open audio output module."); - exit(1); /* communicate failure? */ - } + error("Failed to open audio output module."); + exit(1); /* communicate failure? */ } if(open_output(bao) < 0) { @@ -439,6 +436,25 @@ int init_output(audio_output_t **ao) return 0; } +void exit_output(audio_output_t *ao, int rude) +{ + debug("exit output"); +#ifndef NOXFERMEM + if (param.usebuffer) + { + debug("ending buffer"); + buffer_stop(); /* Puts buffer into waiting-for-command mode. */ + buffer_end(rude); /* Gives command to end operation. */ + xfermem_done_writer(buffermem); + waitpid (buffer_pid, NULL, 0); + xfermem_done (buffermem); + } +#endif + /* Close the output... doesn't matter if buffer handled it, that's taken care of. */ + close_output(ao); + close_output_module(ao); +} + void output_pause(audio_output_t *ao) { if(param.usebuffer) buffer_stop(); @@ -450,17 +466,19 @@ void output_unpause(audio_output_t *ao) if(param.usebuffer) buffer_start(); } -void flush_output(audio_output_t *ao, unsigned char *bytes, size_t count) +int flush_output(audio_output_t *ao, unsigned char *bytes, size_t count) { if(count) { /* Error checks? */ #ifndef NOXFERMEM - if(param.usebuffer) xfermem_write(buffermem, bytes, count); + if(param.usebuffer){ if(xfermem_write(buffermem, bytes, count)) return -1; } else #endif - if(param.outmode != DECODE_TEST) ao->write(ao, bytes, count); + if(param.outmode != DECODE_TEST) + return ao->write(ao, bytes, (int)count); } + return (int)count; /* That is for DECODE_TEST */ } int open_output(audio_output_t *ao) diff --git a/src/audio.h b/src/audio.h index b7b2c7f3..839b8bf1 100644 --- a/src/audio.h +++ b/src/audio.h @@ -77,7 +77,8 @@ int audio_fit_capabilities(audio_output_t *ao,int c,int r); const char* audio_encoding_name(const int encoding, const int longer); int init_output(audio_output_t **ao); -void flush_output(audio_output_t *ao, unsigned char *bytes, size_t count); +void exit_output(audio_output_t *ao, int rude); +int flush_output(audio_output_t *ao, unsigned char *bytes, size_t count); int open_output(audio_output_t *ao); void close_output(audio_output_t *ao ); int reset_output(audio_output_t *ao); diff --git a/src/buffer.c b/src/buffer.c index b3917e9a..5fc8a54e 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -107,7 +107,7 @@ void buffer_sig(int signal, int block) void buffer_loop(audio_output_t *ao, sigset_t *oldsigset) { - int bytes; + int bytes, outbytes; int my_fd = buffermem->fd[XF_READER]; txfermem *xf = buffermem; int done = FALSE; @@ -118,33 +118,31 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset) sigprocmask (SIG_SETMASK, oldsigset, NULL); xfermem_putcmd(my_fd, XF_CMD_WAKEUP); - if(param.outmode == DECODE_AUDIO) + + debug("audio output: waiting for cap requests"); + /* wait for audio setup queries */ + while(1) { - debug("audio output: waiting for cap requests"); - /* wait for audio setup queries */ - while(1) + int cmd; + cmd = xfermem_block(XF_READER, xf); + if(cmd == XF_CMD_AUDIOCAP) { - int cmd; - cmd = xfermem_block(XF_READER, xf); - if(cmd == XF_CMD_AUDIOCAP) - { - ao->rate = xf->rate; - ao->channels = xf->channels; - ao->format = ao->get_formats(ao); - debug3("formats for %liHz/%ich: 0x%x", ao->rate, ao->channels, ao->format); - xf->format = ao->format; - xfermem_putcmd(my_fd, XF_CMD_AUDIOCAP); - } - else if(cmd == XF_CMD_WAKEUP) - { - debug("got wakeup... leaving config mode"); - break; - } - else - { - error1("unexpected command %i", cmd); - return; - } + ao->rate = xf->rate; + ao->channels = xf->channels; + ao->format = ao->get_formats(ao); + debug3("formats for %liHz/%ich: 0x%x", ao->rate, ao->channels, ao->format); + xf->format = ao->format; + xfermem_putcmd(my_fd, XF_CMD_AUDIOCAP); + } + else if(cmd == XF_CMD_WAKEUP) + { + debug("got wakeup... leaving config mode"); + break; + } + else + { + error1("unexpected command %i", cmd); + return; } } @@ -272,18 +270,14 @@ void buffer_loop(audio_output_t *ao, sigset_t *oldsigset) if (bytes > outburst) bytes = outburst; - /* Could change that to use flush_output.... need to capture return value, then. */ -debug("write"); - if (param.outmode == DECODE_FILE) - bytes = write(OutputDescriptor, xf->data + xf->readindex, bytes); - else if (param.outmode == DECODE_AUDIO) - bytes = ao->write(ao, - (unsigned char *) (xf->data + xf->readindex), bytes); + debug("write"); + outbytes = flush_output(ao, (unsigned char*) xf->data + xf->readindex, bytes); - if(bytes < 0) { - bytes = 0; + if(outbytes < bytes) + { + if(outbytes < 0) outbytes = 0; if(!intflag && !usr1flag) { - perror("Ouch ... error while writing audio data: "); + error1("Ouch ... error while writing audio data: %s", strerror(errno)); /* * done==TRUE tells writer process to stop * sending data. There might be some latency @@ -299,6 +293,7 @@ debug("write"); } else debug("buffer interrupted"); } + bytes = outbytes; xf->readindex = (xf->readindex + bytes) % xf->size; if (xf->wakeme[XF_WRITER]) diff --git a/src/mpg123.c b/src/mpg123.c index e55b9402..ad92d909 100644 --- a/src/mpg123.c +++ b/src/mpg123.c @@ -115,6 +115,7 @@ char *prgName = NULL; char *equalfile = NULL; struct httpdata htd; int fresh = TRUE; +int have_output = FALSE; /* If we are past the output init step. */ int buffer_fd[2]; int buffer_pid; @@ -154,7 +155,10 @@ void safe_exit(int code) if(param.term_ctrl) term_restore(); #endif + if(have_output) exit_output(ao, intflag); + if(mh != NULL) mpg123_delete(mh); + mpg123_exit(); httpdata_reset(&htd); /* It's ugly... but let's just fix this still-reachable memory chunk of static char*. */ @@ -524,7 +528,11 @@ int play_frame(void) else print_header_compact(mh); } /* Normal flushing of data, includes buffer decoding. */ - flush_output(ao, audio, bytes); + if(flush_output(ao, audio, bytes) < (int)bytes) + { + error("Deep trouble! Cannot flush to my output anymore!"); + safe_exit(133); + } if(param.checkrange) { long clip = mpg123_clip(mh); @@ -659,6 +667,7 @@ int main(int argc, char *argv[]) mpg123_delete_pars(mp); return 99; /* It's safe here... nothing nasty happened yet. */ } + have_output = TRUE; /* ========================================================================================================= */ /* Enterning the leaking zone... we start messing with stuff here that should be taken care of when leaving. */ @@ -776,8 +785,6 @@ int main(int argc, char *argv[]) if(param.remote) { int ret; ret = control_generic(mh); - close_output(ao); - close_output_module(ao); safe_exit(ret); } @@ -964,23 +971,9 @@ int main(int argc, char *argv[]) #endif } } /* end of loop over input files */ -#ifndef NOXFERMEM - if (param.usebuffer) { - debug("ending buffer"); - buffer_stop(); /* Puts buffer into waiting-for-command mode. */ - buffer_end(intflag); /* Gives command to end operation. */ - xfermem_done_writer (buffermem); - waitpid (buffer_pid, NULL, 0); - xfermem_done (buffermem); - } -#endif - debug("close output"); - /* Close the output... doesn't matter if buffer handled it, that's taken care of. */ - close_output(ao); - close_output_module(ao); /* Free up memory used by playlist */ if(!param.remote) free_playlist(); - safe_exit(0); + safe_exit(0); /* That closes output, too. */ return 0; } diff --git a/src/xfermem.c b/src/xfermem.c index 2d14686e..674838b3 100644 --- a/src/xfermem.c +++ b/src/xfermem.c @@ -226,8 +226,12 @@ int xfermem_write(txfermem *xf, byte *buffer, size_t bytes) /* You weren't so braindead to have not allocated enough space at all, right? */ while (xfermem_get_freespace(xf) < bytes) { - if (xfermem_block(XF_WRITER, xf) == XF_CMD_TERMINATE) - return TRUE; + int cmd = xfermem_block(XF_WRITER, xf); + if (cmd == XF_CMD_TERMINATE || cmd < 0) + { + error("failed to wait for free space"); + return TRUE; /* Failure. */ + } } /* Now we have enough space. copy the memory, possibly with the wrap. */ if(xf->size - xf->freeindex >= bytes) @@ -244,7 +248,8 @@ int xfermem_write(txfermem *xf, byte *buffer, size_t bytes) xf->freeindex = (xf->freeindex + bytes) % xf->size; /* Wake up the buffer process if necessary. */ debug("write waking"); - if(xf->wakeme[XF_READER]) xfermem_putcmd(xf->fd[XF_WRITER], XF_CMD_WAKEUP_INFO); + if(xf->wakeme[XF_READER]) + return xfermem_putcmd(xf->fd[XF_WRITER], XF_CMD_WAKEUP_INFO) < 0 ? TRUE : FALSE; return FALSE; } diff --git a/test/forkfaint.c b/test/forkfaint.c new file mode 100644 index 00000000..274abf7c --- /dev/null +++ b/test/forkfaint.c @@ -0,0 +1,41 @@ +/* + A program that forks mpg123 to read it's output, then faints to release a zombie. + What does mpg123 do? + - pre 1.0rc2: Decode wildly until input end reached, writing into the nonexistent pipe. + - since 1.0rc2: Ending because of I/O error. + +I guess that is what is at the root of the asterisk problem in debug mode. +*/ + +#include +#include +#include +#include +#include + +int main(int argc, char **argv) +{ + pid_t pid; + int strm[2]; + int retval; + if(argc < 2) return -2; + + /* Normally, the decoder gets killed on SIGPIPE when wanting to write to STDOUT, + but with this in place, it gets normal I/O error. */ + signal(SIGPIPE, SIG_IGN); + + pipe(strm); + pid = fork(); + if(pid == 0) + { + close(strm[0]); + dup2(strm[1],STDOUT_FILENO); + fprintf(stderr, "starting decoder\n"); + execlp("mpg123", "mpg123", "-s", "-q", argv[1], NULL); + fprintf(stderr, "Still here? That's bad.(%s)\n", strerror(errno)); + + return -1; + } + printf("Now fainting\n"); + return 0; +}