Re: [PATCH 2/3] ucm: add a FIXME comment

[Takashi Iwai <tiwai@suse.de>]

At Tue, 10 Feb 2015 22:42:33 +0200,
Tanu Kaskinen wrote:
> 
> I'm pretty sure the current code will crash with some inputs, but I
> don't know what the original author intended this code to do, so I
> don't know how to fix it.
> ---
>  src/ucm/main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 3924aee..62bc374 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					uc_error("cdev is not defined!");
>  					return err;
>  				}
> +                                /* FIXME: If cdev1 is NULL and cdev2 is not,
> +                                 * then set cdev to cdev1... makes no sense!
> +                                 * Also, what if both cdev1 and cdev2 are NULL?
> +                                 * What should happen? Later in this function we
> +                                 * call open_ctl(), which assumes non-NULL cdev,
> +                                 * so leaving cdev to NULL here is not an
> +                                 * option (or at least cdev has to be checked
> +                                 * before calling open_ctl()). */
>  				if (cdev1 == NULL || cdev2 == NULL ||
>                                      strcmp(cdev1, cdev2) == 0) {
>  					cdev = (char *)cdev1;

Ouch, the code is really buggy there.  We must fix it instead of
leaving FIXME.

I see there are multiple bugs (in addition to your patch#1):

- the error check is wrong, it should be compared with -ENOENT
	if (err < 0 && err != ENOENT)

  This leaves cdev1 or cdev2 NULL as non-error.

- The intention of the code should be (as far as I understand):
  - if only one of cdev1 and cdev2 is defined, take it as cdev
  - if cdev1 and cdev2 are defined and have the same string, keep
    cdev1 and free cdev2
  - in the rest cases, free both cdev1 and cdev2 without changing cdev


Takashi