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

[Liam Girdwood <liam.r.girdwood@linux.intel.com>]

On Wed, 2015-02-11 at 15:34 +0200, Tanu Kaskinen wrote:
> On Wed, 2015-02-11 at 14:01 +0100, Takashi Iwai wrote:
> > At Wed, 11 Feb 2015 13:44:40 +0200,
> > Tanu Kaskinen wrote:
> > > 
> > > On Wed, 2015-02-11 at 12:38 +0100, Takashi Iwai wrote:
> > > > 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
> > > 
> > > Regarding that last case, just leaving cdev unchanged is not a good
> > > idea. cdev will always be NULL if we don't set it here, and when we call
> > > open_ctl(), it must be non-NULL.
> > 
> > Right.  I *guess* if cdev1 != cdev2, it should bail out as an error.
> 
> Ok, I'll make a patch that will implement the described behaviour.
> 

It may also be worth adding a comment describing the cdev1/2 usage or
maybe even rename them to make it obvious ?

Thanks

Liam