Re: [RFC PATCH 13/27] virtio-snd: Add VIRTIO_SND_R_JACK_INFO handler

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> +    virtio_snd_query_info req;
> +    size_t sz = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req));
> +    assert(sz == sizeof(virtio_snd_query_info));

This assert looks like the guest can trigger it by sending broken
messages.  This should be avoided, the guest should not be able to
kill qemu that way.

> +        jack_info[i - req.start_id].hdr.hda_fn_nid = jack->hda_fn_nid;
> +        jack_info[i - req.start_id].features = jack->features;
> +        jack_info[i - req.start_id].hda_reg_defconf = jack->hda_reg_defconf;
> +        jack_info[i - req.start_id].hda_reg_caps = jack->hda_reg_caps;
> +        jack_info[i - req.start_id].connected = jack->connected;

Disclaimer: didn't check the structs.

If any of these fields is larger than a byte you need to take care of
byte ordering here.  virtio is little endian, so cpu_to_le{16,32}() will
do the job here (if needed).

Same thing elsewhere I suspect.

>          } else if (ctrl.code == VIRTIO_SND_R_JACK_INFO) {
> -            virtio_snd_log("VIRTIO_SND_R_JACK_INFO");
> +            sz = virtio_snd_handle_jack_info(s, elem);
> +            goto done;

Ah, you add the actual command handing here.  Hmm.  I guess a tracepoint
in virtio_snd_handle_jack_info() would be good for debugging.  You could
also log the jack id then.

Also: I'd suggest using "switch(ctrl.code)" here.  Is more readable than
else-if chains (personal opinion though).  Also has the advantage that
gcc will warn in case you forget to handle one of the enums in the
switch.

take care,
  Gerd