Re: [for-8.0 v2 05/11] cryptodev: Introduce 'query-cryptodev' QMP command

[Daniel P . Berrangé]

On Tue, Nov 22, 2022 at 10:07:50PM +0800, zhenwei pi wrote:
> Now we have a QMP command to query crypto devices:
> virsh qemu-monitor-command vm '{"execute": "query-cryptodev"}' | jq
> {
>   "return": [
>     {
>       "service": [
>         "akcipher",
>         "mac",
>         "hash",
>         "cipher"
>       ],
>       "id": "cryptodev1",
>       "client": [
>         {
>           "queue": 0,
>           "type": "builtin",
>           "info": "cryptodev-builtin0"
>         }
>       ]
>     },
>     {
>       "service": [
>         "akcipher"
>       ],
>       "id": "cryptodev0",
>       "client": [
>         {
>           "queue": 0,
>           "type": "lkcf",
>           "info": "cryptodev-lkcf0"
>         }
>       ]
>     }
>   ],
>   "id": "libvirt-415"
> }
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  backends/cryptodev.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  qapi/cryptodev.json  | 43 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index d3caded920..bf2f3234c9 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -24,6 +24,7 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/cryptodev.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-cryptodev.h"
>  #include "qapi/visitor.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
> @@ -33,6 +34,54 @@
>  
>  static QTAILQ_HEAD(, CryptoDevBackendClient) crypto_clients;
>  
> +static int qmp_query_cryptodev_foreach(Object *obj, void *data)
> +{
> +    CryptoDevBackend *backend;
> +    CryptodevInfoList **infolist = data;
> +    uint32_t services;
> +
> +    if (!object_dynamic_cast(obj, TYPE_CRYPTODEV_BACKEND)) {
> +        return 0;
> +    }
> +
> +    CryptodevInfo *info = g_new0(CryptodevInfo, 1);
> +    info->id = g_strdup(object_get_canonical_path_component(obj));
> +
> +    backend = CRYPTODEV_BACKEND(obj);
> +    services = backend->conf.crypto_services;
> +    for (uint32_t i = 0; i < QCRYPTODEV_BACKEND_SERVICE__MAX; i++) {

QEMU coding style doesn't declare types inside the for() control
conditions. I'd suggest 'size_t i', and put it at top of this
function.

> +        if (services & (1 << i)) {
> +            QAPI_LIST_PREPEND(info->service, i);
> +        }
> +    }
> +
> +    for (uint32_t i = 0; i < backend->conf.peers.queues; i++) {
> +        CryptoDevBackendClient *cc = backend->conf.peers.ccs[i];
> +        CryptodevBackendClient *client = g_new0(CryptodevBackendClient, 1);
> +
> +        client->queue = cc->queue_index;
> +        client->type = cc->type;
> +        if (cc->info_str) {
> +            client->has_info = true;
> +            client->info = strdup(cc->info_str);

This will need rebasing, because the 'has_XXXX' fields have gone
away for all pointer types.

> +        }
> +        QAPI_LIST_PREPEND(info->client, client);
> +    }
> +
> +    QAPI_LIST_PREPEND(*infolist, info);
> +
> +    return 0;
> +}
> +
> +CryptodevInfoList *qmp_query_cryptodev(Error **errp)
> +{
> +    CryptodevInfoList *list = NULL;
> +    Object *objs = container_get(object_get_root(), "/objects");
> +
> +    object_child_foreach(objs, qmp_query_cryptodev_foreach, &list);
> +
> +    return list;
> +}
>  
>  CryptoDevBackendClient *cryptodev_backend_new_client(void)
>  {
> diff --git a/qapi/cryptodev.json b/qapi/cryptodev.json
> index 8732a30524..4cc4f4f0ed 100644
> --- a/qapi/cryptodev.json
> +++ b/qapi/cryptodev.json
> @@ -43,3 +43,46 @@
>  { 'enum': 'QCryptodevBackendType',
>    'prefix': 'QCRYPTODEV_BACKEND_TYPE',
>    'data': ['builtin', 'vhost-user', 'lkcf']}
> +
> +##
> +# @CryptodevBackendClient:
> +#
> +# Information about a queue of crypto device.
> +#
> +# @type: the type of the crypto device
> +#
> +# @info: the additional infomation of the crypto device
> +#
> +# Since: 8.0
> +##
> +{ 'struct': 'CryptodevBackendClient',
> +  'data': { 'queue': 'int',
> +            'type': 'QCryptodevBackendType',
> +            '*info': 'str' } }

'queue' field is not documented

I'm not too sure about the approach of exposing 'info'.

It looks like this is either a plain static string whose
value is implicitly determined by 'type', for the 'builtin'
and 'lkcf' backend types, or it is a printf() formattted
string for the 'vhost-user' type, which references the
chardev.

Exposing printf() formatted output is often an anti-pattern
for QAPI design. For example, if it is important for users
to know the chardev assocaited with the vhost-user backend,
then 'info' should be a union that is discriminated by
'type'. The 'vhost-user' branch of the enum should then
identify the chardev 'id' directly.

> +##
> +# @CryptodevInfo:
> +#
> +# Information about a crypto device.
> +#
> +# @service: supported service types of a crypto device
> +#
> +# @client: the additional infomation of the crypto device
> +#
> +# Since: 8.0
> +##
> +{ 'struct': 'CryptodevInfo',
> +  'data': { 'id': 'str',
> +            'service': ['QCryptodevBackendServiceType'],
> +            'client': ['CryptodevBackendClient'] } }

'id' field is not documented.

> +
> +##
> +# @query-cryptodev:
> +#
> +# Returns information about current crypto devices.
> +#
> +# Returns: a list of @CryptodevInfo
> +#
> +# Since: 8.0
> +##
> +{ 'command': 'query-cryptodev', 'returns': ['CryptodevInfo']}

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|