Re: [PATCH v3 1/3] crypto/secret: move main logic from 'secret' to 'secret_common'.

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, May 18, 2020 at 11:28:02PM +0300, Alexey Krasikov wrote:
> Create base class 'common secret'. Move common data and logic from
> 'secret' to 'common_secret' class. This allowed adding abstraction layer
> for easier adding new 'secret' objects in future.
> Convert 'secret' class to child from basic 'secret_common' with 'data'
> and 'file' properties.
> 
> Signed-off-by: Alexey Krasikov <alex-krasikov@yandex-team.ru>
> ---
>  crypto/Makefile.objs           |   1 +
>  crypto/secret.c                | 351 +---------------------------
>  crypto/secret_common.c         | 407 +++++++++++++++++++++++++++++++++
>  include/crypto/secret.h        |  20 +-
>  include/crypto/secret_common.h |  68 ++++++
>  5 files changed, 486 insertions(+), 361 deletions(-)
>  create mode 100644 crypto/secret_common.c
>  create mode 100644 include/crypto/secret_common.h
> 
> diff --git a/crypto/secret_common.c b/crypto/secret_common.c
> new file mode 100644
> index 0000000000..4ef15b98a2
> --- /dev/null
> +++ b/crypto/secret_common.c

> +static void qcrypto_secret_decrypt(QCryptoSecretCommon  *secret,
> +                                   const uint8_t        *input,
> +                                   size_t               inputlen,
> +                                   uint8_t              **output,
> +                                   size_t               *outputlen,
> +                                   Error                **errp)

This has reformatted the original code to vertically line up parameter
names. This is not something we do, so please remove all this extra
whitespace alignment. The is present in other funtions and patches
too, all of which need fixes.


> +static void
> +qcrypto_secret_class_init(ObjectClass *oc, void *data)
> +{
> +    object_class_property_add_bool(oc, "loaded",
> +                                   qcrypto_secret_prop_get_loaded,
> +                                   qcrypto_secret_prop_set_loaded,
> +                                   NULL);
> +    object_class_property_add_enum(oc, "format",
> +                                   "QCryptoSecretFormat",
> +                                   &QCryptoSecretFormat_lookup,
> +                                   qcrypto_secret_prop_get_format,
> +                                   qcrypto_secret_prop_set_format,
> +                                   NULL);
> +    object_class_property_add_str(oc, "keyid",
> +                                  qcrypto_secret_prop_get_keyid,
> +                                  qcrypto_secret_prop_set_keyid,
> +                                  NULL);
> +    object_class_property_add_str(oc, "iv",
> +                                  qcrypto_secret_prop_get_iv,
> +                                  qcrypto_secret_prop_set_iv,
> +                                  NULL);

A patch recently merged which removed the "ERror **errp" parameter
which causes merge conflicts on this patch. You'll just need to
drop the last "NULL" to fix it.


> diff --git a/include/crypto/secret_common.h b/include/crypto/secret_common.h
> new file mode 100644
> index 0000000000..41c06b5391
> --- /dev/null
> +++ b/include/crypto/secret_common.h
> @@ -0,0 +1,68 @@
> +/*
> + * QEMU crypto secret support
> + *
> + * Copyright (c) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef QCRYPTO_SECRET_COMMON_H
> +#define QCRYPTO_SECRET_COMMON_H
> +
> +#include "qapi/qapi-types-crypto.h"
> +#include "qom/object.h"
> +
> +#define TYPE_QCRYPTO_SECRET_COMMON "secret_common"
> +#define QCRYPTO_SECRET_COMMON(obj) \
> +    OBJECT_CHECK(QCryptoSecretCommon, (obj), TYPE_QCRYPTO_SECRET_COMMON)
> +#define QCRYPTO_SECRET_COMMON_CLASS(class) \
> +    OBJECT_CLASS_CHECK(QCryptoSecretCommonClass, \
> +                       (class), TYPE_QCRYPTO_SECRET_COMMON)
> +#define QCRYPTO_SECRET_COMMON_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(QCryptoSecretCommonClass, \
> +                     (obj), TYPE_QCRYPTO_SECRET_COMMON)
> +
> +typedef struct QCryptoSecretCommon QCryptoSecretCommon;
> +typedef struct QCryptoSecretCommonClass QCryptoSecretCommonClass;
> +
> +struct QCryptoSecretCommon {
> +    Object               parent_obj;
> +    uint8_t              *rawdata;
> +    size_t               rawlen;
> +    QCryptoSecretFormat  format;
> +    char                 *keyid;
> +    char                 *iv;
> +};

Don't vertically align struct fields either.


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