| Message ID | 20191119104822.15053-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [alsa-lib,1/4] ucm: Fix opening of master-configs by the card's longname | expand |
Dne 19. 11. 19 v 11:48 Hans de Goede napsal(a): > Prior to commit aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, > keep backward compatibility"). > > The filename build in parse_verb_file() was build like this: > <prefix>/<uc_mgr->conf_file_name>/<file> > > Where uc_mgr->conf_file_name would contain either the card_name or the > card's longname depending on the detection of a longname based config in > uc_mgr_import_master_config(). > > While the filename used in load_master_config() was build like this: > <prefix>/<card_name>/<card_name>.conf > > And uc_mgr_import_master_config() first calls load_master_config() > with the card's longname and if that succeeds it overwrites > uc_mgr->conf_file_name with the longname so that the subsequent uses > of uc_mgr->conf_file_name in parse_verb_file() correctly use the longname. > > But the new configuration_filename() helper added in commit aba2260ae7b5 > _always_ builds the filename like this: > <prefix>/<uc_mgr->conf_file_name>/<file><suffix> > > This breaks the loading of the master-config by its longname, as when > the longname is tried uc_mgr->conf_file_name still contains the card_name. Hi Hans, This new behaviour was intended for ucm2. I applied all four your patches, and added 10a63e093c4a98acfa9bcdfdd06938bcd874b008 on top which should keep the new behaviour for v2 configs. It's time to create the 1.2.1.1 bugfix alsa-lib release, I guess. Thank you, Jaroslav
Hi, On 19-11-2019 13:23, Jaroslav Kysela wrote: > Dne 19. 11. 19 v 11:48 Hans de Goede napsal(a): >> Prior to commit aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, >> keep backward compatibility"). >> >> The filename build in parse_verb_file() was build like this: >> <prefix>/<uc_mgr->conf_file_name>/<file> >> >> Where uc_mgr->conf_file_name would contain either the card_name or the >> card's longname depending on the detection of a longname based config in >> uc_mgr_import_master_config(). >> >> While the filename used in load_master_config() was build like this: >> <prefix>/<card_name>/<card_name>.conf >> >> And uc_mgr_import_master_config() first calls load_master_config() >> with the card's longname and if that succeeds it overwrites >> uc_mgr->conf_file_name with the longname so that the subsequent uses >> of uc_mgr->conf_file_name in parse_verb_file() correctly use the longname. >> >> But the new configuration_filename() helper added in commit aba2260ae7b5 >> _always_ builds the filename like this: >> <prefix>/<uc_mgr->conf_file_name>/<file><suffix> >> >> This breaks the loading of the master-config by its longname, as when >> the longname is tried uc_mgr->conf_file_name still contains the card_name. > > Hi Hans, > > This new behaviour was intended for ucm2. I applied all four your patches, and added 10a63e093c4a98acfa9bcdfdd06938bcd874b008 on top which should keep the > new behaviour for v2 configs. Ah I see. There is a bug in the commit you added on top though, when load_master_config() gets called with longname == 1 and ALSA_CONFIG_UCM2_VAR is not set and ALSA_CONFIG_UCM_VAR is set, then the if condition in the block is false, so it never executes: + if (getenv(ALSA_CONFIG_UCM2_VAR) || !getenv(ALSA_CONFIG_UCM_VAR)) { + uc_mgr->conf_format = 2; + configuration_filename(uc_mgr, filename, sizeof(filename), + uc_mgr->conf_file_name, card_name, ".conf"); + } Causing the next block to also not execute: + if (uc_mgr->conf_format >= 2 && access(filename, R_OK) != 0) { + /* try the old ucm directory */ + uc_mgr->conf_format = 1; + configuration_filename(uc_mgr, filename, sizeof(filename), + card_name, card_name, ".conf"); + if (access(filename, R_OK) != 0) + return -ENOENT; + } Causing load_master_config() to continue with an uninitialized filename. I think you wanted the following condition for the second block: if (uc_mgr->conf_format == 0 || access(filename, R_OK) != 0) { Also don't you want the same behavor wrt forcing the conf_format based on env. when trying the shortname ? I think you can move the entire block under if (longname) to be unconditional, since conf_file_name == card_name for the shortname case using the longname path in the shortname case too is fine. Then you always call configuration_filename with conf_format set and all the handling for conf_format not being set (and for overriding it) can be dropped from configuration_filename() then. > It's time to create the 1.2.1.1 bugfix alsa-lib release, I guess. Yes that is a good idea (once the above is resolved). Regards, Hans
Dne 19. 11. 19 v 13:40 Hans de Goede napsal(a): > Hi, > > On 19-11-2019 13:23, Jaroslav Kysela wrote: >> Dne 19. 11. 19 v 11:48 Hans de Goede napsal(a): >>> Prior to commit aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, >>> keep backward compatibility"). >>> >>> The filename build in parse_verb_file() was build like this: >>> <prefix>/<uc_mgr->conf_file_name>/<file> >>> >>> Where uc_mgr->conf_file_name would contain either the card_name or the >>> card's longname depending on the detection of a longname based config in >>> uc_mgr_import_master_config(). >>> >>> While the filename used in load_master_config() was build like this: >>> <prefix>/<card_name>/<card_name>.conf >>> >>> And uc_mgr_import_master_config() first calls load_master_config() >>> with the card's longname and if that succeeds it overwrites >>> uc_mgr->conf_file_name with the longname so that the subsequent uses >>> of uc_mgr->conf_file_name in parse_verb_file() correctly use the longname. >>> >>> But the new configuration_filename() helper added in commit aba2260ae7b5 >>> _always_ builds the filename like this: >>> <prefix>/<uc_mgr->conf_file_name>/<file><suffix> >>> >>> This breaks the loading of the master-config by its longname, as when >>> the longname is tried uc_mgr->conf_file_name still contains the card_name. >> >> Hi Hans, >> >> This new behaviour was intended for ucm2. I applied all four your patches, and added 10a63e093c4a98acfa9bcdfdd06938bcd874b008 on top which should keep the >> new behaviour for v2 configs. > > Ah I see. > > There is a bug in the commit you added on top though, > when load_master_config() gets called with longname == 1 > and ALSA_CONFIG_UCM2_VAR is not set and ALSA_CONFIG_UCM_VAR > is set, then the if condition in the block is false, so it never executes: > > + if (getenv(ALSA_CONFIG_UCM2_VAR) || !getenv(ALSA_CONFIG_UCM_VAR)) { > + uc_mgr->conf_format = 2; > + configuration_filename(uc_mgr, filename, sizeof(filename), > + uc_mgr->conf_file_name, card_name, ".conf"); > + } > > Causing the next block to also not execute: > > + if (uc_mgr->conf_format >= 2 && access(filename, R_OK) != 0) { > + /* try the old ucm directory */ > + uc_mgr->conf_format = 1; > + configuration_filename(uc_mgr, filename, sizeof(filename), > + card_name, card_name, ".conf"); > + if (access(filename, R_OK) != 0) > + return -ENOENT; > + } > > Causing load_master_config() to continue with an uninitialized filename. > > I think you wanted the following condition for the second block: > > if (uc_mgr->conf_format == 0 || access(filename, R_OK) != 0) { > > Also don't you want the same behavor wrt forcing the conf_format based > on env. when trying the shortname ? > > I think you can move the entire block under if (longname) to be unconditional, > since conf_file_name == card_name for the shortname case using the longname > path in the shortname case too is fine. Then you always call configuration_filename > with conf_format set and all the handling for conf_format not being set (and > for overriding it) can be dropped from configuration_filename() then. Thanks again, fixed in 1d848ab739f0d207098627d2ba66421b3ec75eeb . > >> It's time to create the 1.2.1.1 bugfix alsa-lib release, I guess. > > Yes that is a good idea (once the above is resolved). The tar ball is in ftp://ftp.alsa-project.org/pub/testing . Jaroslav > > Regards, > > Hans >
Hi, On 19-11-2019 14:21, Jaroslav Kysela wrote: > Dne 19. 11. 19 v 13:40 Hans de Goede napsal(a): >> Hi, >> >> On 19-11-2019 13:23, Jaroslav Kysela wrote: >>> Dne 19. 11. 19 v 11:48 Hans de Goede napsal(a): >>>> Prior to commit aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, >>>> keep backward compatibility"). >>>> >>>> The filename build in parse_verb_file() was build like this: >>>> <prefix>/<uc_mgr->conf_file_name>/<file> >>>> >>>> Where uc_mgr->conf_file_name would contain either the card_name or the >>>> card's longname depending on the detection of a longname based config in >>>> uc_mgr_import_master_config(). >>>> >>>> While the filename used in load_master_config() was build like this: >>>> <prefix>/<card_name>/<card_name>.conf >>>> >>>> And uc_mgr_import_master_config() first calls load_master_config() >>>> with the card's longname and if that succeeds it overwrites >>>> uc_mgr->conf_file_name with the longname so that the subsequent uses >>>> of uc_mgr->conf_file_name in parse_verb_file() correctly use the longname. >>>> >>>> But the new configuration_filename() helper added in commit aba2260ae7b5 >>>> _always_ builds the filename like this: >>>> <prefix>/<uc_mgr->conf_file_name>/<file><suffix> >>>> >>>> This breaks the loading of the master-config by its longname, as when >>>> the longname is tried uc_mgr->conf_file_name still contains the card_name. >>> >>> Hi Hans, >>> >>> This new behaviour was intended for ucm2. I applied all four your patches, and added 10a63e093c4a98acfa9bcdfdd06938bcd874b008 on top which should keep the >>> new behaviour for v2 configs. >> >> Ah I see. >> >> There is a bug in the commit you added on top though, >> when load_master_config() gets called with longname == 1 >> and ALSA_CONFIG_UCM2_VAR is not set and ALSA_CONFIG_UCM_VAR >> is set, then the if condition in the block is false, so it never executes: >> >> + if (getenv(ALSA_CONFIG_UCM2_VAR) || !getenv(ALSA_CONFIG_UCM_VAR)) { >> + uc_mgr->conf_format = 2; >> + configuration_filename(uc_mgr, filename, sizeof(filename), >> + uc_mgr->conf_file_name, card_name, ".conf"); >> + } >> >> Causing the next block to also not execute: >> >> + if (uc_mgr->conf_format >= 2 && access(filename, R_OK) != 0) { >> + /* try the old ucm directory */ >> + uc_mgr->conf_format = 1; >> + configuration_filename(uc_mgr, filename, sizeof(filename), >> + card_name, card_name, ".conf"); >> + if (access(filename, R_OK) != 0) >> + return -ENOENT; >> + } >> >> Causing load_master_config() to continue with an uninitialized filename. >> >> I think you wanted the following condition for the second block: >> >> if (uc_mgr->conf_format == 0 || access(filename, R_OK) != 0) { >> >> Also don't you want the same behavor wrt forcing the conf_format based >> on env. when trying the shortname ? >> >> I think you can move the entire block under if (longname) to be unconditional, >> since conf_file_name == card_name for the shortname case using the longname >> path in the shortname case too is fine. Then you always call configuration_filename >> with conf_format set and all the handling for conf_format not being set (and >> for overriding it) can be dropped from configuration_filename() then. > > Thanks again, fixed in 1d848ab739f0d207098627d2ba66421b3ec75eeb . > >> >>> It's time to create the 1.2.1.1 bugfix alsa-lib release, I guess. >> >> Yes that is a good idea (once the above is resolved). > > The tar ball is in ftp://ftp.alsa-project.org/pub/testing . Thanks, I've given this a quick test spin, the 1.2.1.1 tarbal looks good to me. Regards, Hans
On Tue, 19 Nov 2019 16:32:07 +0100, Hans de Goede wrote: > > Hi, > > On 19-11-2019 14:21, Jaroslav Kysela wrote: > > Dne 19. 11. 19 v 13:40 Hans de Goede napsal(a): > >> Hi, > >> > >> On 19-11-2019 13:23, Jaroslav Kysela wrote: > >>> Dne 19. 11. 19 v 11:48 Hans de Goede napsal(a): > >>>> Prior to commit aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, > >>>> keep backward compatibility"). > >>>> > >>>> The filename build in parse_verb_file() was build like this: > >>>> <prefix>/<uc_mgr->conf_file_name>/<file> > >>>> > >>>> Where uc_mgr->conf_file_name would contain either the card_name or the > >>>> card's longname depending on the detection of a longname based config in > >>>> uc_mgr_import_master_config(). > >>>> > >>>> While the filename used in load_master_config() was build like this: > >>>> <prefix>/<card_name>/<card_name>.conf > >>>> > >>>> And uc_mgr_import_master_config() first calls load_master_config() > >>>> with the card's longname and if that succeeds it overwrites > >>>> uc_mgr->conf_file_name with the longname so that the subsequent uses > >>>> of uc_mgr->conf_file_name in parse_verb_file() correctly use the longname. > >>>> > >>>> But the new configuration_filename() helper added in commit aba2260ae7b5 > >>>> _always_ builds the filename like this: > >>>> <prefix>/<uc_mgr->conf_file_name>/<file><suffix> > >>>> > >>>> This breaks the loading of the master-config by its longname, as when > >>>> the longname is tried uc_mgr->conf_file_name still contains the card_name. > >>> > >>> Hi Hans, > >>> > >>> This new behaviour was intended for ucm2. I applied all four your patches, and added 10a63e093c4a98acfa9bcdfdd06938bcd874b008 on top which should keep the > >>> new behaviour for v2 configs. > >> > >> Ah I see. > >> > >> There is a bug in the commit you added on top though, > >> when load_master_config() gets called with longname == 1 > >> and ALSA_CONFIG_UCM2_VAR is not set and ALSA_CONFIG_UCM_VAR > >> is set, then the if condition in the block is false, so it never executes: > >> > >> + if (getenv(ALSA_CONFIG_UCM2_VAR) || !getenv(ALSA_CONFIG_UCM_VAR)) { > >> + uc_mgr->conf_format = 2; > >> + configuration_filename(uc_mgr, filename, sizeof(filename), > >> + uc_mgr->conf_file_name, card_name, ".conf"); > >> + } > >> > >> Causing the next block to also not execute: > >> > >> + if (uc_mgr->conf_format >= 2 && access(filename, R_OK) != 0) { > >> + /* try the old ucm directory */ > >> + uc_mgr->conf_format = 1; > >> + configuration_filename(uc_mgr, filename, sizeof(filename), > >> + card_name, card_name, ".conf"); > >> + if (access(filename, R_OK) != 0) > >> + return -ENOENT; > >> + } > >> > >> Causing load_master_config() to continue with an uninitialized filename. > >> > >> I think you wanted the following condition for the second block: > >> > >> if (uc_mgr->conf_format == 0 || access(filename, R_OK) != 0) { > >> > >> Also don't you want the same behavor wrt forcing the conf_format based > >> on env. when trying the shortname ? > >> > >> I think you can move the entire block under if (longname) to be unconditional, > >> since conf_file_name == card_name for the shortname case using the longname > >> path in the shortname case too is fine. Then you always call configuration_filename > >> with conf_format set and all the handling for conf_format not being set (and > >> for overriding it) can be dropped from configuration_filename() then. > > > > Thanks again, fixed in 1d848ab739f0d207098627d2ba66421b3ec75eeb . > > > >> > >>> It's time to create the 1.2.1.1 bugfix alsa-lib release, I guess. > >> > >> Yes that is a good idea (once the above is resolved). > > > > The tar ball is in ftp://ftp.alsa-project.org/pub/testing . > > Thanks, I've given this a quick test spin, the 1.2.1.1 tarbal looks good to me. Jaroslav, when will this tarball released officially? thanks, Takashi
Dne 25. 11. 19 v 11:12 Takashi Iwai napsal(a): >>> The tar ball is in ftp://ftp.alsa-project.org/pub/testing . >> >> Thanks, I've given this a quick test spin, the 1.2.1.1 tarbal looks good to me. > > Jaroslav, when will this tarball released officially? Oops, it's already in /pub/lib on ALSA's FTP. I just forget to write a release note. I will do it now. Jaroslav
diff --git a/src/ucm/parser.c b/src/ucm/parser.c index ed790dc0..17aab054 100644 --- a/src/ucm/parser.c +++ b/src/ucm/parser.c @@ -76,9 +76,10 @@ static void configuration_filename2(char *fn, size_t fn_len, int format, static void configuration_filename(snd_use_case_mgr_t *uc_mgr, char *fn, size_t fn_len, - const char *file, const char *suffix) + const char *dir, const char *file, + const char *suffix) { - const char *env, *dir; + const char *env; if (uc_mgr->conf_format > 0) { /* known format */ @@ -94,13 +95,11 @@ static void configuration_filename(snd_use_case_mgr_t *uc_mgr, } } if (env) { - snprintf(fn, fn_len, "%s/%s/%s%s", - env, uc_mgr->conf_file_name, file, suffix); + snprintf(fn, fn_len, "%s/%s/%s%s", env, dir, file, suffix); fn[fn_len-1] = '\0'; return; } - dir = uc_mgr->conf_file_name; if (uc_mgr->conf_format > 0) { __format: configuration_filename2(fn, fn_len, uc_mgr->conf_format, @@ -1181,7 +1180,8 @@ static int parse_verb_file(snd_use_case_mgr_t *uc_mgr, } /* open Verb file for reading */ - configuration_filename(uc_mgr, filename, sizeof(filename), file, ""); + configuration_filename(uc_mgr, filename, sizeof(filename), + uc_mgr->conf_file_name, file, ""); err = uc_mgr_config_load(uc_mgr->conf_format, filename, &cfg); if (err < 0) { uc_error("error: failed to open verb file %s : %d", @@ -1576,7 +1576,7 @@ static int load_master_config(snd_use_case_mgr_t *uc_mgr, } configuration_filename(uc_mgr, filename, sizeof(filename), - card_name, ".conf"); + card_name, card_name, ".conf"); /* if the configuration file does not exist, silently return */ if (fcheck && access(filename, R_OK) != 0)
Prior to commit aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, keep backward compatibility"). The filename build in parse_verb_file() was build like this: <prefix>/<uc_mgr->conf_file_name>/<file> Where uc_mgr->conf_file_name would contain either the card_name or the card's longname depending on the detection of a longname based config in uc_mgr_import_master_config(). While the filename used in load_master_config() was build like this: <prefix>/<card_name>/<card_name>.conf And uc_mgr_import_master_config() first calls load_master_config() with the card's longname and if that succeeds it overwrites uc_mgr->conf_file_name with the longname so that the subsequent uses of uc_mgr->conf_file_name in parse_verb_file() correctly use the longname. But the new configuration_filename() helper added in commit aba2260ae7b5 _always_ builds the filename like this: <prefix>/<uc_mgr->conf_file_name>/<file><suffix> This breaks the loading of the master-config by its longname, as when the longname is tried uc_mgr->conf_file_name still contains the card_name. This commit fixes this by adding a dir parameter to configuration_filename() and restoring the old behavior by passing card_name as dir in load_master_config(). Fixes: aba2260ae7b5 ("ucm: switch to ucm2 directory and v2 format, keep backward compatibility") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- src/ucm/parser.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)