From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 2/3] ucm: add a FIXME comment Date: Wed, 11 Feb 2015 12:38:56 +0100 Message-ID: References: <1423600954-8751-1-git-send-email-tanu.kaskinen@linux.intel.com> <1423600954-8751-3-git-send-email-tanu.kaskinen@linux.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 8184B260570 for ; Wed, 11 Feb 2015 12:38:57 +0100 (CET) In-Reply-To: <1423600954-8751-3-git-send-email-tanu.kaskinen@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Tanu Kaskinen Cc: liam.r.girdwood@linux.intel.com, alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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