[Spice-devel] [RFC] register vmc interface early for name != vdagent [was: Re: Read data out of the Virtqueue]

[Hans de Goede]

Hi,

On 04/06/2012 09:50 AM, Charles.Tsai-蔡清海-研究發展部 wrote:
> Hans,
>
> This approach did not work and I gave it up already.

Charles, since that approach is already used in usb-redir
devices which are already in use by quite a few people I assure
you that it does work!

You are likely doing something wrong in your code somewhere, it
would help tremendously if you could simply post your qemu patches
here, or publish a git tree with your code.

We understand that the code in question will be a work in progress.
That is not a problem, and it simply is much much easier to discuss
these kind of things when we've code to look at.

Regards,

Hans



>
> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede at redhat.com]
> Sent: Friday, April 06, 2012 3:50 PM
> To: Charles.Tsai-蔡清海-研究發展部; spice-devel at lists.freedesktop.org
> Subject: Re: [Spice-devel] [RFC] register vmc interface early for name != vdagent [was: Re: Read data out of the Virtqueue]
>
> Hi,
>
> On 04/05/2012 05:36 PM, Alon Levy wrote:
>> On Thu, Apr 05, 2012 at 06:22:47AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
>>> Alon,
>>>
>>> 	Can you tell me more specific point what you meant " bottomhalf"?
>>>
>>> 	I am testing my modified code to send a message thru the main channel and it works for me.
>>> 	However, if there is a better approach to fix this problem, I will take it.
>>
>> Something like this:
>>
>>
>> diff --git a/spice-qemu-char.c b/spice-qemu-char.c index
>> 1e735ff..560127e 100644
>> --- a/spice-qemu-char.c
>> +++ b/spice-qemu-char.c
>> @@ -25,6 +25,7 @@ typedef struct SpiceCharDriver {
>>        uint8_t               *datapos;
>>        ssize_t               bufsize, datalen;
>>        uint32_t              debug;
>> +    QEMUBH                *bh_register;
>>    } SpiceCharDriver;
>>
>>    static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t
>> *buf, int len) @@ -188,6 +189,14 @@ static void print_allowed_subtypes(void)
>>        fprintf(stderr, "\n");
>>    }
>>
>> +static void bh_vmc_register_interface(void *opaque) {
>> +    SpiceCharDriver *s = opaque;
>> +
>> +    vmc_register_interface(s);
>> +    qemu_bh_delete(s->bh_register);
>> +}
>> +
>>    CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
>>    {
>>        CharDriverState *chr;
>> @@ -226,12 +235,15 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
>>        chr->chr_guest_open = spice_chr_guest_open;
>>        chr->chr_guest_close = spice_chr_guest_close;
>>
>> -#if SPICE_SERVER_VERSION<   0x000901
>> -    /* See comment in vmc_state() */
>>        if (strcmp(subtype, "vdagent") == 0) {
>> +#if SPICE_SERVER_VERSION<   0x000901
>> +        /* See comment in vmc_state() */
>>            qemu_chr_generic_open(chr);
>> -    }
>>    #endif
>> +    } else {
>> +        s->bh_register = qemu_bh_new(bh_vmc_register_interface, s);
>> +        qemu_bh_schedule(s->bh_register);
>> +    }
>>
>>        return chr;
>>    }
>>
>
>
> Please don't do this. We've an established mechanism for registering the chardev with spice-server, we do this when the chardev frontend signals us it is ready. It does this through the guest_open / close chardev callbacks. These are somewhat poorly named since their origin lies in virtio_console where they indeed reflect opening of the device by the guest.
>
> But in essence they mean the chardev frontend is ready to receive data.
>
> Notice that although the callbacks still have bad names, the way to call them from a frontend has much better names now a days:
> qemu_chr_fe_open / qemu_chr_fe_close
>
> Any special virtual printer frontend should call qemu_chr_fe_open from its qdev init function (like the usb redirection code does) and then the chardev will get registered with spice-server, and this will happen after the chardev has been registered.
>
> The above code is very clearly wrong since it simply just always register the chardev backend with the spice-server, potentially before the frontend has called qemu_chr_fe_open(), BAD!
>
> More as the above shows using a bh is not always a magic solution to problems. See for example also this commit message where using a bh was specifically avoided:
> http://git.qemu.org/?p=qemu.git;a=commitdiff;h=f76e4c7f16c7ab966a792310b6630d3e240688b3
>
> Regards,
>
> Hans