On Mon, May 09, 2022 at 12:19:07PM +0200, Paolo Bonzini wrote: > The ``loaded=on`` option in the command line or QMP ``object-add`` either had > no effect (if ``loaded`` was the last option) or caused options to be > effectively ignored as if they were not given. The property is therefore > useless and was deprecated in 6.0; make it read-only now.
Why read-only, as opposed to deleting it entirely ? Unless I'm missing something, nothing will read the property either > > The patch is best reviewed with "-b". > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > --- > crypto/secret_common.c | 84 ++++++++++++++------------------- > crypto/tlscredsanon.c | 20 ++------ > crypto/tlscredspsk.c | 20 ++------ > crypto/tlscredsx509.c | 20 ++------ > docs/about/deprecated.rst | 10 ---- > docs/about/removed-features.rst | 8 ++++ > 6 files changed, 55 insertions(+), 107 deletions(-) > > diff --git a/crypto/secret_common.c b/crypto/secret_common.c > index 714a15d5e5..3441c44ca8 100644 > --- a/crypto/secret_common.c > +++ b/crypto/secret_common.c > @@ -138,36 +138,44 @@ static void qcrypto_secret_decode(const uint8_t *input, > > > static void > -qcrypto_secret_prop_set_loaded(Object *obj, > - bool value, > - Error **errp) > +qcrypto_secret_complete(UserCreatable *uc, Error **errp) > { > - QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj); > + QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(uc); > QCryptoSecretCommonClass *sec_class > - = QCRYPTO_SECRET_COMMON_GET_CLASS(obj); > + = QCRYPTO_SECRET_COMMON_GET_CLASS(uc); > > - if (value) { > - Error *local_err = NULL; > - uint8_t *input = NULL; > - size_t inputlen = 0; > - uint8_t *output = NULL; > - size_t outputlen = 0; > + Error *local_err = NULL; > + uint8_t *input = NULL; > + size_t inputlen = 0; > + uint8_t *output = NULL; > + size_t outputlen = 0; > > - if (sec_class->load_data) { > - sec_class->load_data(secret, &input, &inputlen, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - } else { > - error_setg(errp, "%s provides no 'load_data' method'", > - object_get_typename(obj)); > + if (sec_class->load_data) { > + sec_class->load_data(secret, &input, &inputlen, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > return; > } > + } else { > + error_setg(errp, "%s provides no 'load_data' method'", > + object_get_typename(OBJECT(uc))); > + return; > + } > > - if (secret->keyid) { > - qcrypto_secret_decrypt(secret, input, inputlen, > - &output, &outputlen, &local_err); > + if (secret->keyid) { > + qcrypto_secret_decrypt(secret, input, inputlen, > + &output, &outputlen, &local_err); > + g_free(input); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + input = output; > + inputlen = outputlen; > + } else { > + if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) { > + qcrypto_secret_decode(input, inputlen, > + &output, &outputlen, &local_err); > g_free(input); > if (local_err) { > error_propagate(errp, local_err); > @@ -175,26 +183,11 @@ qcrypto_secret_prop_set_loaded(Object *obj, > } > input = output; > inputlen = outputlen; > - } else { > - if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) { > - qcrypto_secret_decode(input, inputlen, > - &output, &outputlen, &local_err); > - g_free(input); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > - input = output; > - inputlen = outputlen; > - } > } > - > - secret->rawdata = input; > - secret->rawlen = inputlen; > - } else if (secret->rawdata) { > - error_setg(errp, "Cannot unload secret"); > - return; > } > + > + secret->rawdata = input; > + secret->rawlen = inputlen; > } > > > @@ -268,13 +261,6 @@ qcrypto_secret_prop_get_keyid(Object *obj, > } > > > -static void > -qcrypto_secret_complete(UserCreatable *uc, Error **errp) > -{ > - object_property_set_bool(OBJECT(uc), "loaded", true, errp); > -} > - > - > static void > qcrypto_secret_finalize(Object *obj) > { > @@ -294,7 +280,7 @@ 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, > diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c > index 6fb83639ec..c0d23a0ef3 100644 > --- a/crypto/tlscredsanon.c > +++ b/crypto/tlscredsanon.c > @@ -119,16 +119,11 @@ qcrypto_tls_creds_anon_unload(QCryptoTLSCredsAnon > *creds G_GNUC_UNUSED) > > > static void > -qcrypto_tls_creds_anon_prop_set_loaded(Object *obj, > - bool value, > - Error **errp) > +qcrypto_tls_creds_anon_complete(UserCreatable *uc, Error **errp) > { > - QCryptoTLSCredsAnon *creds = QCRYPTO_TLS_CREDS_ANON(obj); > + QCryptoTLSCredsAnon *creds = QCRYPTO_TLS_CREDS_ANON(uc); > > - qcrypto_tls_creds_anon_unload(creds); > - if (value) { > - qcrypto_tls_creds_anon_load(creds, errp); > - } > + qcrypto_tls_creds_anon_load(creds, errp); > } > > > @@ -163,13 +158,6 @@ qcrypto_tls_creds_anon_prop_get_loaded(Object *obj > G_GNUC_UNUSED, > #endif /* ! CONFIG_GNUTLS */ > > > -static void > -qcrypto_tls_creds_anon_complete(UserCreatable *uc, Error **errp) > -{ > - object_property_set_bool(OBJECT(uc), "loaded", true, errp); > -} > - > - > static void > qcrypto_tls_creds_anon_finalize(Object *obj) > { > @@ -188,7 +176,7 @@ qcrypto_tls_creds_anon_class_init(ObjectClass *oc, void > *data) > > object_class_property_add_bool(oc, "loaded", > qcrypto_tls_creds_anon_prop_get_loaded, > - qcrypto_tls_creds_anon_prop_set_loaded); > + NULL); > } > > > diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c > index 752f2d92be..a4f9891274 100644 > --- a/crypto/tlscredspsk.c > +++ b/crypto/tlscredspsk.c > @@ -188,16 +188,11 @@ qcrypto_tls_creds_psk_unload(QCryptoTLSCredsPSK *creds > G_GNUC_UNUSED) > > > static void > -qcrypto_tls_creds_psk_prop_set_loaded(Object *obj, > - bool value, > - Error **errp) > +qcrypto_tls_creds_psk_complete(UserCreatable *uc, Error **errp) > { > - QCryptoTLSCredsPSK *creds = QCRYPTO_TLS_CREDS_PSK(obj); > + QCryptoTLSCredsPSK *creds = QCRYPTO_TLS_CREDS_PSK(uc); > > - qcrypto_tls_creds_psk_unload(creds); > - if (value) { > - qcrypto_tls_creds_psk_load(creds, errp); > - } > + qcrypto_tls_creds_psk_load(creds, errp); > } > > > @@ -232,13 +227,6 @@ qcrypto_tls_creds_psk_prop_get_loaded(Object *obj > G_GNUC_UNUSED, > #endif /* ! CONFIG_GNUTLS */ > > > -static void > -qcrypto_tls_creds_psk_complete(UserCreatable *uc, Error **errp) > -{ > - object_property_set_bool(OBJECT(uc), "loaded", true, errp); > -} > - > - > static void > qcrypto_tls_creds_psk_finalize(Object *obj) > { > @@ -276,7 +264,7 @@ qcrypto_tls_creds_psk_class_init(ObjectClass *oc, void > *data) > > object_class_property_add_bool(oc, "loaded", > qcrypto_tls_creds_psk_prop_get_loaded, > - qcrypto_tls_creds_psk_prop_set_loaded); > + NULL); > object_class_property_add_str(oc, "username", > qcrypto_tls_creds_psk_prop_get_username, > qcrypto_tls_creds_psk_prop_set_username); > diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c > index 32948a6bdc..d14313925d 100644 > --- a/crypto/tlscredsx509.c > +++ b/crypto/tlscredsx509.c > @@ -687,16 +687,11 @@ qcrypto_tls_creds_x509_unload(QCryptoTLSCredsX509 > *creds G_GNUC_UNUSED) > > > static void > -qcrypto_tls_creds_x509_prop_set_loaded(Object *obj, > - bool value, > - Error **errp) > +qcrypto_tls_creds_x509_complete(UserCreatable *uc, Error **errp) > { > - QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj); > + QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(uc); > > - qcrypto_tls_creds_x509_unload(creds); > - if (value) { > - qcrypto_tls_creds_x509_load(creds, errp); > - } > + qcrypto_tls_creds_x509_load(creds, errp); > } > > > @@ -814,13 +809,6 @@ qcrypto_tls_creds_x509_reload(QCryptoTLSCreds *creds, > Error **errp) > #endif /* ! CONFIG_GNUTLS */ > > > -static void > -qcrypto_tls_creds_x509_complete(UserCreatable *uc, Error **errp) > -{ > - object_property_set_bool(OBJECT(uc), "loaded", true, errp); > -} > - > - > static void > qcrypto_tls_creds_x509_init(Object *obj) > { > @@ -852,7 +840,7 @@ qcrypto_tls_creds_x509_class_init(ObjectClass *oc, void > *data) > > object_class_property_add_bool(oc, "loaded", > qcrypto_tls_creds_x509_prop_get_loaded, > - qcrypto_tls_creds_x509_prop_set_loaded); > + NULL); > object_class_property_add_bool(oc, "sanity-check", > qcrypto_tls_creds_x509_prop_get_sanity, > qcrypto_tls_creds_x509_prop_set_sanity); > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 896e5a97ab..2feb0c506c 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -99,16 +99,6 @@ other options have been processed. This will either have > no effect (if > ``opened`` was the last option) or cause errors. The property is therefore > useless and should not be specified. > > -``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0) > -'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > - > -The only effect of specifying ``loaded=on`` in the command line or QMP > -``object-add`` is that the secret is loaded immediately, possibly before all > -other options have been processed. This will either have no effect (if > -``loaded`` was the last option) or cause options to be effectively ignored as > -if they were not given. The property is therefore useless and should not be > -specified. > - > ``-display sdl,window_close=...`` (since 6.1) > ''''''''''''''''''''''''''''''''''''''''''''' > > diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst > index 4a0b270296..2032608314 100644 > --- a/docs/about/removed-features.rst > +++ b/docs/about/removed-features.rst > @@ -741,6 +741,14 @@ aware that there are already potential security risks to > blindly using > ``qemu-img info`` to probe the format of an untrusted backing image, > when deciding what format to add into an existing image. > > +``loaded`` property of ``secret`` and ``secret_keyring`` objects (removed in > 7.1) > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +The ``loaded=on`` option in the command line or QMP ``object-add`` either had > +no effect (if ``loaded`` was the last option) or caused options to be > +effectively ignored as if they were not given. The property is therefore > +useless and should simply be removed. > + > Block devices > ------------- > > -- > 2.35.1 > 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 :|