Re: [Qemu-devel] [PATCH] Initial implementation of a mpeg1 layer2 streaming audio driver.

[malc <av1474@comtv.ru>]

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