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

[Hans de Goede]

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