Re: [Qemu-devel] [PATCH v4 05/13] virtio-crypto: add virtio crypto device emulation

[Stefan Hajnoczi <stefanha@redhat.com>]

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

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.

> +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

> +        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().

> +#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
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)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]