Zhuoying Cai <zy...@linux.ibm.com> writes:
> On 6/18/25 1:57 AM, Markus Armbruster wrote:
>> Zhuoying Cai <zy...@linux.ibm.com> writes:
>>
>>> On 6/17/25 6:58 AM, Markus Armbruster wrote:
>>>> Zhuoying Cai <zy...@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 <zy...@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.
>> Similar questions for the other QAPI enums added in this series.
