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