Re: [PATCH v2 11/31] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'

[Kevin Wolf <kwolf@redhat.com>]

Am 26.02.2021 um 20:33 hat Eric Blake geschrieben:
> On 2/24/21 7:52 AM, Kevin Wolf wrote:
> > This adds a QAPI schema for the properties of the tls-* objects.
> > 
> > The 'loaded' property doesn't seem to make sense as an external
> > interface: It is automatically set to true in ucc->complete, and
> > explicitly setting it to true earlier just means that additional options
> > will be silently ignored.
> > 
> > In other words, the 'loaded' property is useless. Mark it as deprecated
> > in the schema from the start.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/crypto.json | 98 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi/qom.json    | 12 +++++-
> >  2 files changed, 108 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index 0fef3de66d..7116ae9a46 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -442,3 +442,101 @@
> >  { 'struct': 'SecretKeyringProperties',
> >    'base': 'SecretCommonProperties',
> >    'data': { 'serial': 'int32' } }
> > +
> > +##
> > +# @TlsCredsProperties:
> > +#
> > +# Properties for objects of classes derived from tls-creds.
> > +#
> > +# @verify-peer: if true the peer credentials will be verified once the
> > +#               handshake is completed.  This is a no-op for anonymous
> > +#               credentials. (default: true)
> > +#
> > +# @dir: the path of the directory that contains the credential files
> > +#
> > +# @endpoint: whether the QEMU network backend that uses the credentials will be
> > +#            acting as a client or as a server (default: client)
> > +#
> > +# @priority: a gnutls priority string as described at
> > +#            https://gnutls.org/manual/html_node/Priority-Strings.html
> > +#
> > +# Since: 2.5
> > +##
> > +{ 'struct': 'TlsCredsProperties',
> > +  'data': { '*verify-peer': 'bool',
> > +            '*dir': 'str',
> > +            '*endpoint': 'QCryptoTLSCredsEndpoint',
> > +            '*priority': 'str' } }
> 
> Matches crypto/tlscreds.c:qcrypto_tls_creds_class_init().
> 
> > +
> > +##
> > +# @TlsCredsAnonProperties:
> > +#
> > +# Properties for tls-creds-anon objects.
> > +#
> > +# @loaded: if true, the credentials are loaded immediately when applying this
> > +#          option and will ignore options that are processed later. Don't use;
> > +#          only provided for compatibility. (default: false)
> > +#
> > +# Features:
> > +# @deprecated: Member @loaded is deprecated.  Setting true doesn't make sense,
> > +#              and false is already the default.
> > +#
> > +# Since: 2.5
> > +##
> > +{ 'struct': 'TlsCredsAnonProperties',
> > +  'base': 'TlsCredsProperties',
> > +  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] } } }
> 
> Since we documented that 'verify-peer' is a no-op for this struct, is it
> worth altering our type hierarchy to make it explicit, as in:
> 
> TlsCredsCommonProperties - dir, endpoint, priority
> TlsCredsProperties - TlsCredsCommonProperties + verify-peer
> TlsCredsAnonProperties - TlsCredsCommonProperties + loaded
> TlsCredsPskProperties - TlsCredsProperties + loaded, username
> 
> But even if not, this matches
> crypto/tlscredsanon.c:qcrypto_tls_creds_anon_class_init().

We can't turn a no-op into an error without a deprecation period.

> > +
> > +##
> > +# @TlsCredsPskProperties:
> > +#
> > +# Properties for tls-creds-psk objects.
> > +#
> > +# @loaded: if true, the credentials are loaded immediately when applying this
> > +#          option and will ignore options that are processed later. Don't use;
> > +#          only provided for compatibility. (default: false)
> > +#
> > +# @username: the username which will be sent to the server.  For clients only.
> > +#            If absent, "qemu" is sent and the property will read back as an
> > +#            empty string.
> > +#
> > +# Features:
> > +# @deprecated: Member @loaded is deprecated.  Setting true doesn't make sense,
> > +#              and false is already the default.
> > +#
> > +# Since: 3.0
> > +##
> > +{ 'struct': 'TlsCredsPskProperties',
> > +  'base': 'TlsCredsProperties',
> > +  'data': { '*loaded': { 'type': 'bool', 'features': ['deprecated'] },
> > +            '*username': 'str' } }
> 
> This matches crypto/tlscredspsk.c:qcrypto_tls_creds_psk_class_init().
> 
> Do we want to use QAPI type inheritance to declare a union where
> 'endpoint' is the union discriminator, and 'username' is only present
> for 'endpoint':'client'?  (Hmm, we'd have to improve the QAPI code
> generator to allow a flat union as the branch of yet another flat
> union...)

Probably not now then.

It also has the same problem as above, but I guess you could use the
deprecation period to build the required QAPI infrastructure. :-)

Kevin