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

[François Revol]

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 ?

>> +
>> +    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...

>> +
>> +        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



>> +    }
>> +    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.