RE: [PATCH v4 05/13] virtio-crypto: add virtio crypto device emulation

["Gonglei (Arei)" <arei.gonglei@huawei.com>]

> From: Stefan Hajnoczi [mailto:stefanha@redhat.com]
> Sent: Tuesday, October 04, 2016 5:38 PM
> Subject: Re: [PATCH v4 05/13] virtio-crypto: add virtio crypto device emulation
> 
> On Wed, Sep 28, 2016 at 04:25:44PM +0800, Gonglei wrote:
> > Introduce the virtio crypto realization, I'll
> > finish the core code in the following patches. The
> > thoughts came from virtio net realization.
> >
> > For more information see:
> > http://qemu-project.org/Features/VirtioCrypto
> 
> This patch contains functions and struct fields that are unused.  It
> would make code review easier if you add them when they are needed
> throughout the patch series.
> 
OK, will remove them.

> > +static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +    VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(dev);
> > +    int i;
> > +
> > +    vcrypto->cryptodev = vcrypto->conf.cryptodev;
> > +    if (vcrypto->cryptodev == NULL) {
> > +        error_setg(errp, "'cryptodev' parameter expects a valid object");
> > +        return;
> > +    }
> > +
> > +    vcrypto->max_queues = MAX(vcrypto->cryptodev->conf.peers.queues,
> 1);
> > +    if (vcrypto->max_queues + 1 > VIRTIO_QUEUE_MAX) {
> > +        error_setg(errp, "Invalid number of queues (= %" PRIu16 "), "
> > +                   "must be a postive integer less than %d.",
> > +                   vcrypto->max_queues, VIRTIO_QUEUE_MAX - 1);
> 
> The error message is off by 1:
> 
> must be a positive integer less than VIRTIO_QUEUE_MAX
> 
Yes.

> > +        return;
> > +    }
> > +
> > +    virtio_init(vdev, "virtio-crypto", VIRTIO_ID_CRYPTO,
> vcrypto->config_size);
> > +    vcrypto->curr_queues = 1;
> > +
> > +    for (i = 0; i < vcrypto->max_queues; i++) {
> > +        virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq);
> > +    }
> > +
> > +    vcrypto->ctrl_vq = virtio_add_queue(vdev, 64,
> virtio_crypto_handle_ctrl);
> > +    if (!vcrypto->cryptodev->ready) {
> > +        vcrypto->status &= ~VIRTIO_CRYPTO_S_HW_READY;
> > +    } else {
> > +        vcrypto->status |= VIRTIO_CRYPTO_S_HW_READY;
> > +    }
> > +    register_savevm(dev, "virtio-crypto", -1, 1, virtio_crypto_save,
> > +                    virtio_crypto_load, vcrypto);
> 
> Please use VMSTATE_VIRTIO_DEVICE() instead of calling register_savevm().
> 
OK.

> > +#ifdef DEBUG_VIRTIO_CRYPTO
> > +#define DPRINTF(fmt, ...) \
> > +do { printf("virtio_crypto: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do { } while (0)
> > +#endif
> 
> Please use tracing (see docs/tracing.txt) or if you really want to use

I planned to polish tracing log in my TODO list. So, currently I'd like to use
DPRINTF().

> debug printfs then define DPRINTF() in a way that prevents bitrot:
> 
> #define DEBUG_VIRTIO_CRYPTO 0
> #define DPRINTF(fmt, ...) \
> do { \
>     if (DEBUG_VIRTIO_CRYPTO) { \
>         fprintf(stderr, "virtio_crypto: " fmt, ##__VA_ARGS__); \
>     } \
> } while (0)

Make sense.


Regards,
-Gonglei