Re: [PATCH v3 02/28] crypto/x509-utils: Add helper functions for certificate store

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

On Mon, Jun 23, 2025 at 08:15:16AM +0200, Markus Armbruster wrote:
> Zhuoying Cai <zycai@linux.ibm.com> writes:
> 
> > On 6/18/25 1:57 AM, Markus Armbruster wrote:
> >> Zhuoying Cai <zycai@linux.ibm.com> writes:
> >> 
> >>> On 6/17/25 6:58 AM, Markus Armbruster wrote:
> >>>> Zhuoying Cai <zycai@linux.ibm.com> writes:
> >>>>
> >>>>> Add helper functions for x509 certificate which will be used in the next
> >>>>> patch for the certificate store.
> >>>>>
> >>>>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> >> 
> >> [...]
> >> 
> >>>> Ignorant question: why are these QAPI enums?
> >>>>
> >>>> If they need to be QAPI enums, then I'll have some requests on the doc
> >>>> comments.
> >>>>
> >>>
> >>> Hi, thanks for the feedback.
> >>>
> >>> The helper functions in x509-utils.c either take QAPI enum values as
> >>> parameters or return them. These enums are used later within QEMU.
> >> 
> >> Let's look at the first one I found:
> >> 
> >>     int qcrypto_check_x509_cert_fmt(uint8_t *cert, size_t size,
> >>                                     QCryptoCertFmt fmt, Error **errp)
> >>     {
> >>         int rc;
> >>         int ret = -1;
> >>         gnutls_x509_crt_t crt;
> >>         gnutls_datum_t datum = {.data = cert, .size = size};
> >> 
> >>         if (fmt >= G_N_ELEMENTS(qcrypto_to_gnutls_cert_fmt_map)) {
> >>             error_setg(errp, "Unknown certificate format");
> >>             return ret;
> >>         }
> >> 
> >>         if (gnutls_x509_crt_init(&crt) < 0) {
> >>             error_setg(errp, "Failed to initialize certificate");
> >>             return ret;
> >>         }
> >> 
> >>         rc = gnutls_x509_crt_import(crt, &datum, qcrypto_to_gnutls_cert_fmt_map[fmt]);
> >>         if (rc == GNUTLS_E_ASN1_TAG_ERROR) {
> >>             goto cleanup;
> >>         }
> >> 
> >>         ret = 0;
> >> 
> >>     cleanup:
> >>         gnutls_x509_crt_deinit(crt);
> >>         return ret;
> >>     }
> >> 
> >> All it does with its @fmt argument is map it to the matching
> >> GNUTLS_X509_FMT_*.
> >> 
> >> There's just one caller, init_cert_x509_der() in hw/s390x/cert-store.c:
> >> 
> >>     is_der = qcrypto_check_x509_cert_fmt((uint8_t *)raw, size,
> >>                                          QCRYPTO_CERT_FMT_DER, &err);
> >> 
> >> QCRYPTO_CERT_FMT_DER gets mapped to GNUTLS_X509_FMT_DER.  Why not pass
> >> that directly?  We don't need enum QCryptoCertFmt then.
> >> 
> >
> > I received feedback on a previous patch series that directly using
> > GNUTLS in QEMU code is discouraged, except for under the crypto/
> > directory. Internal APIs should be defined to access GNUTLS
> > functionality instead.
> >
> >> If we need enum QCryptoCertFmt for some reason I can't see, why does it
> >> have to be a QAPI type?  Why not a plain C enum?
> >
> > While implementing the new helper functions, I referred to
> > qcrypto_get_x509_cert_fingerprint() in crypto/x509-utils.c, which takes
> > QCryptoHashAlgo as a parameter. Following this, I added corresponding
> > QCRYPTO enums to map to GNUTLS enums.
> >
> > If using plain C enums is preferred, I can update the code accordingly
> > in the next version.
> 
> Use plain C enums when practical.
> 
> Reasons for making a type a QAPI type include:
> 
> * Some QAPI command or event needs it.
> 
> * Something (typically QOM property accessors) needs the generated
>   visitor.
> 
> * For enums: something could use the generated QEnumLookup / ENUM_str()
>   macro.

Any time a method only accepts a subset of enum values, and needs to report
an error message, it should always use ENUM_str in the error message, rather
than the raw int value, to make the error message human friendly and invariant
to enum ordering.  IOW, ENUM_str should be relatively frequently used/needed.

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