| Message ID | 20200510043240.1631106-2-eschwartz@archlinux.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [pacman-dev,1/3] pacman-conf: add support for new ParallelDownloads config option | expand |
On 10/5/20 2:32 pm, Eli Schwartz wrote: > This is not a warning, _parse_options() returns failure without even > parsing further lines and the attempted pacman/pacman-conf program > execution immediately aborts. Warnings are for when e.g. later on if we > don't recognize a setting at all, we skip over it and have enough > confidence in this to continue executing the program. > > The current implementation results in pacman-conf aborting with: > > warning: config file /etc/pacman.conf, line 60: invalid value for 'ParallelDownloads' : '2.5' > error parsing '/etc/pacman.conf' > > or pacman -Syu aborting with the entirely more cryptic: > > warning: config file /etc/pacman.conf, line 59: invalid value for 'ParallelDownloads' : '2.5' > > and this isn't just a problem for the newly added ParallelDownloads > setting, either, you could get the same problem if you specified a > broken XferCommand, but that's harder as it's more accepting of input > and you probably don't hit this except with unbalanced quotes. > Is this the fix we want? I think it would be better to get to the end of parsing the conf file spitting errors as we go so that all issues are printed before exiting. Allan > Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> > --- > src/pacman/conf.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/pacman/conf.c b/src/pacman/conf.c > index ac5a5329..3a3ef605 100644 > --- a/src/pacman/conf.c > +++ b/src/pacman/conf.c > @@ -665,7 +665,7 @@ static int _parse_options(const char *key, char *value, > } else if(strcmp(key, "XferCommand") == 0) { > char **c; > if((config->xfercommand_argv = wordsplit(value)) == NULL) { > - pm_printf(ALPM_LOG_WARNING, > + pm_printf(ALPM_LOG_ERROR, > _("config file %s, line %d: invalid value for '%s' : '%s'\n"), > file, linenum, "XferCommand", value); > return 1; > @@ -717,21 +717,21 @@ static int _parse_options(const char *key, char *value, > > err = parse_number(value, &number); > if(err) { > - pm_printf(ALPM_LOG_WARNING, > + pm_printf(ALPM_LOG_ERROR, > _("config file %s, line %d: invalid value for '%s' : '%s'\n"), > file, linenum, "ParallelDownloads", value); > return 1; > } > > if(number < 1) { > - pm_printf(ALPM_LOG_WARNING, > + pm_printf(ALPM_LOG_ERROR, > _("config file %s, line %d: value for '%s' has to be positive : '%s'\n"), > file, linenum, "ParallelDownloads", value); > return 1; > } > > if(number > INT_MAX) { > - pm_printf(ALPM_LOG_WARNING, > + pm_printf(ALPM_LOG_ERROR, > _("config file %s, line %d: value for '%s' is too large : '%s'\n"), > file, linenum, "ParallelDownloads", value); > return 1; >
On 5/10/20 4:09 AM, Allan McRae wrote: > On 10/5/20 2:32 pm, Eli Schwartz wrote: >> This is not a warning, _parse_options() returns failure without even >> parsing further lines and the attempted pacman/pacman-conf program >> execution immediately aborts. Warnings are for when e.g. later on if we >> don't recognize a setting at all, we skip over it and have enough >> confidence in this to continue executing the program. >> >> The current implementation results in pacman-conf aborting with: >> >> warning: config file /etc/pacman.conf, line 60: invalid value for 'ParallelDownloads' : '2.5' >> error parsing '/etc/pacman.conf' >> >> or pacman -Syu aborting with the entirely more cryptic: >> >> warning: config file /etc/pacman.conf, line 59: invalid value for 'ParallelDownloads' : '2.5' >> >> and this isn't just a problem for the newly added ParallelDownloads >> setting, either, you could get the same problem if you specified a >> broken XferCommand, but that's harder as it's more accepting of input >> and you probably don't hit this except with unbalanced quotes. >> > > Is this the fix we want? I think it would be better to get to the end > of parsing the conf file spitting errors as we go so that all issues are > printed before exiting. > > Allan Maybe? All I have done thus far is change the log level. To collect multiple errors I guess we'd need to stop aborting in ini.c:parse_ini the first time one callback returns an error. It's currently documented with: @note Parsing will immediately stop if the callback returns non-zero.
On 10/5/20 6:22 pm, Eli Schwartz wrote: > On 5/10/20 4:09 AM, Allan McRae wrote: >> On 10/5/20 2:32 pm, Eli Schwartz wrote: >>> This is not a warning, _parse_options() returns failure without even >>> parsing further lines and the attempted pacman/pacman-conf program >>> execution immediately aborts. Warnings are for when e.g. later on if we >>> don't recognize a setting at all, we skip over it and have enough >>> confidence in this to continue executing the program. >>> >>> The current implementation results in pacman-conf aborting with: >>> >>> warning: config file /etc/pacman.conf, line 60: invalid value for 'ParallelDownloads' : '2.5' >>> error parsing '/etc/pacman.conf' >>> >>> or pacman -Syu aborting with the entirely more cryptic: >>> >>> warning: config file /etc/pacman.conf, line 59: invalid value for 'ParallelDownloads' : '2.5' >>> >>> and this isn't just a problem for the newly added ParallelDownloads >>> setting, either, you could get the same problem if you specified a >>> broken XferCommand, but that's harder as it's more accepting of input >>> and you probably don't hit this except with unbalanced quotes. >>> >> >> Is this the fix we want? I think it would be better to get to the end >> of parsing the conf file spitting errors as we go so that all issues are >> printed before exiting. >> >> Allan > > Maybe? All I have done thus far is change the log level. To collect > multiple errors I guess we'd need to stop aborting in ini.c:parse_ini > the first time one callback returns an error. > > It's currently documented with: > > > @note Parsing will immediately stop if the callback returns non-zero. > OK - I'll take your patch as is, and we may want to consider that later. We also may have issues continuing to parse after a failure. A
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index ac5a5329..3a3ef605 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -665,7 +665,7 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "XferCommand") == 0) { char **c; if((config->xfercommand_argv = wordsplit(value)) == NULL) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: invalid value for '%s' : '%s'\n"), file, linenum, "XferCommand", value); return 1; @@ -717,21 +717,21 @@ static int _parse_options(const char *key, char *value, err = parse_number(value, &number); if(err) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: invalid value for '%s' : '%s'\n"), file, linenum, "ParallelDownloads", value); return 1; } if(number < 1) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: value for '%s' has to be positive : '%s'\n"), file, linenum, "ParallelDownloads", value); return 1; } if(number > INT_MAX) { - pm_printf(ALPM_LOG_WARNING, + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: value for '%s' is too large : '%s'\n"), file, linenum, "ParallelDownloads", value); return 1;
This is not a warning, _parse_options() returns failure without even parsing further lines and the attempted pacman/pacman-conf program execution immediately aborts. Warnings are for when e.g. later on if we don't recognize a setting at all, we skip over it and have enough confidence in this to continue executing the program. The current implementation results in pacman-conf aborting with: warning: config file /etc/pacman.conf, line 60: invalid value for 'ParallelDownloads' : '2.5' error parsing '/etc/pacman.conf' or pacman -Syu aborting with the entirely more cryptic: warning: config file /etc/pacman.conf, line 59: invalid value for 'ParallelDownloads' : '2.5' and this isn't just a problem for the newly added ParallelDownloads setting, either, you could get the same problem if you specified a broken XferCommand, but that's harder as it's more accepting of input and you probably don't hit this except with unbalanced quotes. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- src/pacman/conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)