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

[François Revol]

Le 8 nov. 2010 à 01:02, malc a écrit :

>> +
>> +static void qtwolame_listen_read(void *opaque)
> 
> No space here.

?

>> +    if (csock != -1) {
>> +        twolame->sock = csock;
>> +        dolog ("Accepted peer\n");
>> +        if (write (twolame->sock, http_header, sizeof(http_header) - 1) < 
>> sizeof(http_header) - 1) {
> 
> Line is too long.

Dang, wrote it after the 80col pass.

> 
>> +            qtwolame_logerr (errno, "write failed for http headers\n");
>> +            /* sending headers failed, just close the connection */
>> +            closesocket (twolame->sock);
>> +            twolame->sock = -1;
>> +        }
>> +    }
> 
> twolame->csock is not set to -1 (the condition which is checked for 
> everywhere)

There is no twolame->csock, just twolame->lsock (the listen socket), and 
twolame->sock (=csock temporary var) which is the accepted socket.

>> +        again:
>> +            if (twolame->sock > -1) {
>> +                written = write (twolame->sock, twolame->mpg_buf, 
>> converted);
>> +                if (written == -1) {
>> +                    if (errno == EPIPE) {
>> +                        dolog ("Lost peer\n");
>> +                        closesocket (twolame->sock);
> 
> This is actually no better than jumping before, reading this code requires
> more analysis than needed, and no i'm not saying it's not correct just hard
> to read.

I'll have to sleep on this then.

>> +    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, conf.rate);
>> +
>> +    if (twolame_init_params(twolame->options)) {
>> +        dolog ("Could not set twolame options\n");
>> +        goto fail1;
>> +    }
> 
> Once again, if you don't like the space before paren by all means do not use
> it, but either way do things consistently.

Ah, forgot this one.

>> +    qemu_set_fd_handler2(twolame->lsock, NULL, NULL, NULL, NULL);
>> +    if (closesocket (twolame->lsock)) {
> 
> Here we go again, closesocket is cheked here but not elsewhere.

Because elsewhere it's done in an error path and it'd won't do much better, but 
oh well.


> And can you, please, elaborate some more on usage scenarios of this thing?


cf.
http://dev.haiku-os.org/browser/haiku/trunk/3rdparty/mmu_man/onlinedemo/haiku.php

and possibly http://oszoo.org/wiki/index.php/Main_Page some day...

The idea is to use it along with the -vnc option and the VNC applet to present 
a VM on the web.

François.