Re: [Qemu-devel] [PATCH v5 3/3] ivshmem: add check on protocol version in QEMU

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On Thu, Sep 04, 2014 at 02:51:01PM +0200, David Marchand wrote:
> diff --git a/contrib/ivshmem-client/ivshmem-client.c b/contrib/ivshmem-client/ivshmem-client.c
> index ad210c8..0c4e016 100644
> --- a/contrib/ivshmem-client/ivshmem-client.c
> +++ b/contrib/ivshmem-client/ivshmem-client.c
> @@ -184,10 +184,18 @@ ivshmem_client_connect(IvshmemClient *client)
>          goto err_close;
>      }
>  
> -    /* first, we expect our index + a fd == -1 */
> +    /* first, we expect a protocol version */
> +    if (read_one_msg(client, &tmp, &fd) < 0 ||
> +        (tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) {
> +        debug_log(client, "cannot read from server\n");
> +        goto err_close;
> +    }
> +    debug_log(client, "our_id=%ld\n", client->local.id);

This debug_log() is probably not intentional.  local.id will always be
-1 here so the output is not useful.

> +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int flags)
> +{
> +    IVShmemState *s = opaque;
> +    PCIDevice *dev = PCI_DEVICE(s);
> +    int tmp;
> +    long version;
> +
> +    memcpy(&version, buf, sizeof(long));
> +    tmp = qemu_chr_fe_get_msgfd(s->server_chr);
> +    if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) {
> +        fprintf(stderr, "incompatible version, you are connecting to a ivhsmem-"
> +                "server using a different protocol please check your setup\n");
> +        qemu_chr_delete(s->server_chr);
> +        s->server_chr = NULL;
> +        return;
> +    }
> +
> +    IVSHMEM_DPRINTF("version check ok, finish init and switch to real chardev "
> +                    "handler\n");
> +
> +    pci_register_bar(dev, 2, s->ivshmem_attr, &s->bar);

Not sure if it is okay to delay PCI initialization to a fd hander
callback.

If the version message is too slow the guest could see the PCI adapter
without the BAR!

Did you move this code in order to prevent the guest from accessing the
device before it has connected to the server?  Perhaps the device needs
a state field that tracks whether or not it is ready for operation.  Any
access before RUNNING state is reached will be ignored (?).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]