| Message ID | 1469084447-30740-1-git-send-email-mengdong.lin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Jul 21 2016 16:00, mengdong.lin@linux.intel.com wrote: > From: Mengdong Lin <mengdong.lin@linux.intel.com> > > tplg_copy_data() should set the valid referenced data element pointer. > The caller will double check this pointer for all kinds of references, > including TLV, controls, text and data. > > Also fix an inaccurate dmesg. > > Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com> > > diff --git a/src/topology/dapm.c b/src/topology/dapm.c > index 4d343b2..e3c90d8 100644 > --- a/src/topology/dapm.c > +++ b/src/topology/dapm.c > @@ -201,7 +201,7 @@ static int tplg_build_widget(snd_tplg_t *tplg, > } > > if (!ref->elem) { > - SNDERR("error: cannot find control '%s'" > + SNDERR("error: cannot find '%s'" > " referenced by widget '%s'\n", > ref->id, elem->id); > return -EINVAL; It's better to split to two patches for the different aims. Accumulating patches to log history is preferable, as long as you have concrete wills to improve codes, I believe. > diff --git a/src/topology/data.c b/src/topology/data.c > index 768fc27..397f6a5 100644 > --- a/src/topology/data.c > +++ b/src/topology/data.c > @@ -1050,6 +1050,7 @@ int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, > " element '%s'\n", ref->id, elem->id); > return -EINVAL; > } > + ref->elem = ref_elem; > > tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id); > /* overlook empty private data */ I wonder why the assignment is done in this line. This function still includes some codes to return errors. In this case, it's better to code the assignment in last part of the function. This is a public API. It's better not to touch application's objects in invalid cases. Regards Takashi Sakamoto
On Jul 22 2016 05:37, Takashi Sakamoto wrote: > Hi, > > On Jul 21 2016 16:00, mengdong.lin@linux.intel.com wrote: >> From: Mengdong Lin <mengdong.lin@linux.intel.com> >> >> tplg_copy_data() should set the valid referenced data element pointer. >> The caller will double check this pointer for all kinds of references, >> including TLV, controls, text and data. >> >> Also fix an inaccurate dmesg. >> >> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com> >> >> diff --git a/src/topology/dapm.c b/src/topology/dapm.c >> index 4d343b2..e3c90d8 100644 >> --- a/src/topology/dapm.c >> +++ b/src/topology/dapm.c >> @@ -201,7 +201,7 @@ static int tplg_build_widget(snd_tplg_t *tplg, >> } >> >> if (!ref->elem) { >> - SNDERR("error: cannot find control '%s'" >> + SNDERR("error: cannot find '%s'" >> " referenced by widget '%s'\n", >> ref->id, elem->id); >> return -EINVAL; > > It's better to split to two patches for the different aims. Accumulating > patches to log history is preferable, as long as you have concrete wills > to improve codes, I believe. > >> diff --git a/src/topology/data.c b/src/topology/data.c >> index 768fc27..397f6a5 100644 >> --- a/src/topology/data.c >> +++ b/src/topology/data.c >> @@ -1050,6 +1050,7 @@ int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, >> " element '%s'\n", ref->id, elem->id); >> return -EINVAL; >> } >> + ref->elem = ref_elem; >> >> tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id); >> /* overlook empty private data */ > > I wonder why the assignment is done in this line. This function still > includes some codes to return errors. In this case, it's better to code > the assignment in last part of the function. > > This is a public API. It's better not to touch application's objects in > invalid cases. Oops. I overlooked. It's in src/topology/tplg_local.h and not a public API... Anyway, it's better to consider about the error cases for consistency of object state. Regards Takashi Sakamoto
> -----Original Message----- > From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp] > Sent: Friday, July 22, 2016 5:16 AM > > It's better to split to two patches for the different aims. > > Accumulating patches to log history is preferable, as long as you have > > concrete wills to improve codes, I believe. Okay. I split them in v2. > > > >> diff --git a/src/topology/data.c b/src/topology/data.c index > >> 768fc27..397f6a5 100644 > >> --- a/src/topology/data.c > >> +++ b/src/topology/data.c > >> @@ -1050,6 +1050,7 @@ int tplg_copy_data(snd_tplg_t *tplg, struct > tplg_elem *elem, > >> " element '%s'\n", ref->id, elem->id); > >> return -EINVAL; > >> } > >> + ref->elem = ref_elem; > >> > >> tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id); > >> /* overlook empty private data */ > > > > I wonder why the assignment is done in this line. This function still > > includes some codes to return errors. In this case, it's better to > > code the assignment in last part of the function. > > > > This is a public API. It's better not to touch application's objects > > in invalid cases. > > Oops. I overlooked. It's in src/topology/tplg_local.h and not a public API... > Anyway, it's better to consider about the error cases for consistency of object > state. Yes. It's better to move to the end of this function, only on success. Also fixed this in v2 series "topology: Fix for handling widgets' references" Thanks for your review! Mengdong
diff --git a/src/topology/dapm.c b/src/topology/dapm.c index 4d343b2..e3c90d8 100644 --- a/src/topology/dapm.c +++ b/src/topology/dapm.c @@ -201,7 +201,7 @@ static int tplg_build_widget(snd_tplg_t *tplg, } if (!ref->elem) { - SNDERR("error: cannot find control '%s'" + SNDERR("error: cannot find '%s'" " referenced by widget '%s'\n", ref->id, elem->id); return -EINVAL; diff --git a/src/topology/data.c b/src/topology/data.c index 768fc27..397f6a5 100644 --- a/src/topology/data.c +++ b/src/topology/data.c @@ -1050,6 +1050,7 @@ int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, " element '%s'\n", ref->id, elem->id); return -EINVAL; } + ref->elem = ref_elem; tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id); /* overlook empty private data */