[Spice-devel] [cacard 6/7] cac-aca: Favour g_return_val_if_fail over g_assert

[Frediano Ziglio]

> 
> Some g_assert are used to sanity-check input arguments of some methods.
> g_return_val_if_fail() is more appropriate for that as it won't cause
> the program using libcacard to terminate. The callers of these methods
> are already checking the return value for NULL.
> 

I personally think g_assert is the proper way to check that functions
do not accept invalid objects.
I'll prefer the usage of attribute(nonnull) and tools like coverity
instead.
The fact that function callers check for NULL at the end does not mean
that the functions can return NULL on invalid input, means that some
error can lead to NULL as result of the execution of these function,
passing NULL is an error of callers.

> Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> ---
>  src/cac-aca.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cac-aca.c b/src/cac-aca.c
> index a915689..1d86a74 100644
> --- a/src/cac-aca.c
> +++ b/src/cac-aca.c
> @@ -168,7 +168,7 @@ cac_aca_get_acr(size_t *acr_len, unsigned char *acrid)
>      size_t i;
>      int j = 0;
>  
> -    g_assert_nonnull(acr_len);
> +    g_return_val_if_fail(acr_len != NULL, NULL);
>  
>      if (acrid != NULL) {
>          r = g_malloc(sizeof(struct simpletlv_member));
> @@ -325,7 +325,7 @@ cac_aca_get_service_table(size_t *r_len, unsigned int
> pki_applets)
>      size_t i, j = 0;
>      unsigned int num_entries;
>  
> -    g_assert_nonnull(r_len);
> +    g_return_val_if_fail(r_len != NULL, NULL);
>  
>      num_entries = service_table.num_static_entries + pki_applets;
>      r = g_malloc_n(num_entries + 1, sizeof(struct simpletlv_member));
> @@ -859,7 +859,7 @@ cac_aca_get_applet_acr(unsigned int pki_applets, size_t
> *acr_len, unsigned char
>      unsigned char applet_id = 0;
>      unsigned int num_applets = applets_table.num_static_applets +
>      pki_applets;
>  
> -    g_assert_nonnull(acr_len);
> +    g_return_val_if_fail(acr_len != NULL, NULL);
>  
>      if (aid != NULL && aid_len != 0) {
>          /* We are selecting only one applet*/
> @@ -969,7 +969,7 @@ cac_aca_get_amp(size_t *amp_len)
>      unsigned char *entry = NULL;
>      size_t i = 0;
>  
> -    g_assert_nonnull(amp_len);
> +    g_return_val_if_fail(amp_len != NULL, NULL);
>  
>      r = g_malloc_n(amp_table.num_entries + 1, sizeof(struct
>      simpletlv_member));
>  
> @@ -1014,7 +1014,8 @@ static struct simpletlv_member aca_properties[1] = {
>  static struct simpletlv_member *
>  cac_aca_get_properties(size_t *properties_len)
>  {
> -    g_assert_nonnull(properties_len);
> +    /* FIXME: callers don't handle NULL return value, but it can't happen
> anyway */
> +    g_return_val_if_fail(properties_len != NULL, NULL);
> 

This new regression does not make sense to me.
 
>      /* Inject Applet Version into Applet information */
>      aca_properties[0].value.value = applet_information;

Personally nacked

Frediano