Re: [PATCH v2 07/25] s390x/diag: Implement DIAG 320 subcode 2

[Thomas Huth <thuth@redhat.com>]

On 29/05/2025 21.09, Zhuoying Cai wrote:
> 
> On 5/14/25 12:18 PM, Thomas Huth wrote:
>> On 09/05/2025 00.50, Zhuoying Cai wrote:
...
>>> +int qcrypto_get_x509_cert_key_id(uint8_t *cert, size_t size,
>>> +                                 QCryptoKeyidFlags flag,
>>> +                                 uint8_t *result,
>>> +                                 size_t *resultlen,
>>> +                                 Error **errp)
>>> +{
>>> +    int ret = -1;
>>> +    int keyid_len;
>>> +    gnutls_x509_crt_t crt;
>>> +    gnutls_datum_t datum = {.data = cert, .size = size};
>>> +    gnutls_x509_crt_fmt_t fmt;
>>> +
>>> +    if (flag >= G_N_ELEMENTS(qcrypto_to_gnutls_keyid_flags_map)) {
>>> +        error_setg(errp, "Unknown key id flag");
>>> +        return -1;
>>> +    }
>>> +
>>> +    if (result == NULL) {
>>> +        error_setg(errp, "No valid buffer given");
>>> +        return -1;
>>> +    }
>>
>> This check sounds like it could also be a simple g_assert() statement instead?
>>
> 
> g_assert() may not be ideal here, as it will terminate the guest if the
> assertion fails, which is not the intended behavior.

OK, but if it is valid to call this function with result == NULL, then this 
does not sound like an error, thus you likely should not use error_setg() 
here. If it *not* valid to call this function with result == NULL, then 
there is a programming error and it is OK to use g_assert() here.
Anyway, please add a comment in front of the function where you describe the 
parameters, including the information whether the pointers are allowed to be 
NULL or not.

  Thomas