From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=47411 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFA4z-00069m-NQ for qemu-devel@nongnu.org; Sun, 07 Nov 2010 13:35:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PFA4y-0007G1-92 for qemu-devel@nongnu.org; Sun, 07 Nov 2010 13:35:01 -0500 Received: from fe01x03-cgp.akado.ru ([77.232.31.164]:55980 helo=akado.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PFA4y-0007Fq-0y for qemu-devel@nongnu.org; Sun, 07 Nov 2010 13:35:00 -0500 Date: Sun, 7 Nov 2010 21:34:53 +0300 (MSK) From: malc Subject: Re: [Qemu-devel] [PATCH] Initial implementation of a mpeg1 layer2 streaming audio driver. In-Reply-To: <2946F11E-81BD-4AE3-B537-7BE5960A2650@free.fr> Message-ID: References: <1EDFDC6F-1836-4870-A5B3-47ED41060224@free.fr> <2946F11E-81BD-4AE3-B537-7BE5960A2650@free.fr> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-15?Q?Fran=E7ois_Revol?= Cc: qemu-devel@nongnu.org On Sun, 7 Nov 2010, Fran?ois Revol wrote: > > Le 7 nov. 2010 ? 19:09, malc a ?crit : > > > On Sun, 7 Nov 2010, Fran?ois Revol wrote: > > > > Please CC audio related stuff to audio maintainer. > > And that'd be you according to MAINTAINERS ? > > >> +static const char http_header[] = "HTTP/1.1 200 OK\r\nServer: QEMU\r\nContent-Type: audio/mpeg\r\n\r\n"; > > > > Line is too long. > > Ok I'll break at 80col. > > >> + if (twolame->fd > -1) > >> + return; > > > > Style. > > That is ? Braces around statements. > >> + > >> + int csock = qemu_accept(twolame->lsock, (struct sockaddr *)&addr, &addrlen); > > > > C99 intermixed declartion and initialization is not allowed. > > This line I copied form ui/vnc.c which does violate C89 too btw... I do not maintain ui/vnc.c. > > >> + > >> + again: > >> + if (twolame->fd > -1) { > >> + written = write (twolame->fd, twolame->mpg_buf, converted); > >> + if (written == -1) { > >> + if (errno == EPIPE) { > >> + dolog ("Lost peer\n"); > >> + close (twolame->fd); > >> + twolame->fd = -1; > >> + goto again; > > > > This goto is obfuscated. > > Not much more than in esdaudio.c > Yes much more, esdaudio doesn't close the descriptor before jumping. > > >> + } > >> + obt_as.endianness = AUDIO_HOST_ENDIANNESS; > >> + > >> + audio_pcm_init_info (&hw->info, &obt_as); > >> + > >> + twolame_set_mode(twolame->options, (as->nchannels == 2) ? TWOLAME_STEREO : TWOLAME_MONO); > >> + twolame_set_num_channels(twolame->options, as->nchannels); > >> + twolame_set_in_samplerate(twolame->options, as->freq); > >> + twolame_set_out_samplerate(twolame->options, as->freq); > >> + twolame_set_bitrate(twolame->options, 160); //XXX:conf. > >> + > >> + if (twolame_init_params(twolame->options)) { > >> + dolog ("Could not set twolame options\n"); > >> + return -1; > >> + } > > > > Inconsistent space before opening paren. > > Sorry, used to the Haiku style without space before but it seemed to be > different around. > > > >> + twolame->mpg_buf = audio_calloc (AUDIO_FUNC, hw->samples, 1 << hw->info.shift); > >> + if (!twolame->mpg_buf) { > > > > pcm_buf is not freed. > > > >> + > >> +// fail1: > > > > Do not use C99 style comments. > > Oh that's leftover from copied error handling, which isn't correct anyway. > > Fran?ois. > -- mailto:av1474@comtv.ru