[pacman-dev] [PATCH 1/3] pacman-conf: add support for new ParallelDownloads config option

older
[pacman-dev] [GIT] The official...

Eli Schwartz

10 May 2020 10 May '20

4:32 a.m.

This was forgotten in the initial implementation, so it was impossible to figure out the value from a script, or correctly roundtrip the config file. Signed-off-by: Eli Schwartz --- src/pacman/pacman-conf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 60739bf6..d76c0985 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -142,6 +142,14 @@ static void show_str(const char *directive, const char *val) printf("%s%c", val, sep); } +static void show_int(const char *directive, unsigned int val) +{ + if (verbose) { + printf("%s = ", directive); + } + printf("%u%c", val, sep); +} + static void show_list_str(const char *directive, alpm_list_t *list) { alpm_list_t *i; @@ -261,6 +269,8 @@ static void dump_config(void) show_bool("ILoveCandy", config->chomp); show_bool("NoProgressBar", config->noprogressbar); + show_int("ParallelDownloads", config->parallel_downloads); + show_cleanmethod("CleanMethod", config->cleanmethod); show_siglevel("SigLevel", config->siglevel, 0); @@ -372,6 +382,9 @@ static int list_directives(void) } else if(strcasecmp(i->data, "NoProgressBar") == 0) { show_bool("NoProgressBar", config->noprogressbar); + } else if(strcasecmp(i->data, "ParallelDownloads") == 0) { + show_int("ParallelDownloads", config->parallel_downloads); + } else if(strcasecmp(i->data, "CleanMethod") == 0) { show_cleanmethod("CleanMethod", config->cleanmethod); -- 2.26.2

Show replies by date

Eli Schwartz

10 May 10 May

4:32 a.m.

New subject: [pacman-dev] [PATCH 2/3] log invalid conf settings as an error

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 --- 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; -- 2.26.2

Allan McRae

8:09 a.m.

New subject: [pacman-dev] [PATCH 2/3] log invalid conf settings as an error

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 --- 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;

Eli Schwartz

8:22 a.m.

New subject: [pacman-dev] [PATCH 2/3] log invalid conf settings as an error

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. -- Eli Schwartz Bug Wrangler and Trusted User

Allan McRae

8:41 a.m.

New subject: [pacman-dev] [PATCH 2/3] log invalid conf settings as an error

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

Eli Schwartz

4:32 a.m.

New subject: [pacman-dev] [PATCH 3/3] pacman-conf: fix half-baked support for ILoveCandy

This was only partially implemented in the original implementation. `pacman-conf | grep ILoveCandy` would tell you if it was set, but querying directly by name would not. Signed-off-by: Eli Schwartz --- src/pacman/pacman-conf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index d76c0985..463badf1 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -379,6 +379,8 @@ static int list_directives(void) show_bool("VerbosePkgLists", config->verbosepkglists); } else if(strcasecmp(i->data, "DisableDownloadTimeout") == 0) { show_bool("DisableDownloadTimeout", config->disable_dl_timeout); + } else if(strcasecmp(i->data, "ILoveCandy") == 0) { + show_bool("ILoveCandy", config->chomp); } else if(strcasecmp(i->data, "NoProgressBar") == 0) { show_bool("NoProgressBar", config->noprogressbar); -- 2.26.2

Allan McRae

8:06 a.m.

New subject: [pacman-dev] [PATCH 3/3] pacman-conf: fix half-baked support for ILoveCandy

On 10/5/20 2:32 pm, Eli Schwartz wrote:

...

This was only partially implemented in the original implementation. `pacman-conf | grep ILoveCandy` would tell you if it was set, but querying directly by name would not.

OK. Note "half-baked" can have negative connotations, while "incomplete" specifies that issue without being accusatory.

...

Signed-off-by: Eli Schwartz --- src/pacman/pacman-conf.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index d76c0985..463badf1 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -379,6 +379,8 @@ static int list_directives(void) show_bool("VerbosePkgLists", config->verbosepkglists); } else if(strcasecmp(i->data, "DisableDownloadTimeout") == 0) { show_bool("DisableDownloadTimeout", config->disable_dl_timeout); + } else if(strcasecmp(i->data, "ILoveCandy") == 0) { + show_bool("ILoveCandy", config->chomp); } else if(strcasecmp(i->data, "NoProgressBar") == 0) { show_bool("NoProgressBar", config->noprogressbar);

Eli Schwartz

8:11 a.m.

New subject: [pacman-dev] [PATCH 3/3] pacman-conf: fix half-baked support for ILoveCandy

On 5/10/20 4:06 AM, Allan McRae wrote:

...

On 10/5/20 2:32 pm, Eli Schwartz wrote:

...
This was only partially implemented in the original implementation. `pacman-conf | grep ILoveCandy` would tell you if it was set, but querying directly by name would not.

OK.

Note "half-baked" can have negative connotations, while "incomplete" specifies that issue without being accusatory. Feel free to exchange the commit message with that text, and chalk it up to late-night coding. :D

-- Eli Schwartz Bug Wrangler and Trusted User

Allan McRae

8:04 a.m.

On 10/5/20 2:32 pm, Eli Schwartz wrote:

...

This was forgotten in the initial implementation, so it was impossible to figure out the value from a script, or correctly roundtrip the config file.

Signed-off-by: Eli Schwartz --- src/pacman/pacman-conf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)

OK.

...

diff --git a/src/pacman/pacman-conf.c b/src/pacman/pacman-conf.c index 60739bf6..d76c0985 100644 --- a/src/pacman/pacman-conf.c +++ b/src/pacman/pacman-conf.c @@ -142,6 +142,14 @@ static void show_str(const char *directive, const char *val) printf("%s%c", val, sep); }

+static void show_int(const char *directive, unsigned int val) +{ + if (verbose) { + printf("%s = ", directive); + } + printf("%u%c", val, sep); +} + static void show_list_str(const char *directive, alpm_list_t *list) { alpm_list_t *i; @@ -261,6 +269,8 @@ static void dump_config(void) show_bool("ILoveCandy", config->chomp); show_bool("NoProgressBar", config->noprogressbar);

+ show_int("ParallelDownloads", config->parallel_downloads); + show_cleanmethod("CleanMethod", config->cleanmethod);

show_siglevel("SigLevel", config->siglevel, 0); @@ -372,6 +382,9 @@ static int list_directives(void) } else if(strcasecmp(i->data, "NoProgressBar") == 0) { show_bool("NoProgressBar", config->noprogressbar);

+ } else if(strcasecmp(i->data, "ParallelDownloads") == 0) { + show_int("ParallelDownloads", config->parallel_downloads); + } else if(strcasecmp(i->data, "CleanMethod") == 0) { show_cleanmethod("CleanMethod", config->cleanmethod);

1448

Age (days ago)

1448

Last active (days ago)

List overview

Download

8 comments

2 participants

participants (2)

Allan McRae
Eli Schwartz

[pacman-dev] [PATCH 1/3] pacman-conf: add support for new ParallelDownloads config option

tags

participants (2)