[pacman-dev] [PATCH 1/2] skip unneeded parsing
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them) Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com> --- src/pacman/conf.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 7ba2791..09749ea 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -755,6 +755,12 @@ static int _parseconfig(const char *file, struct section_t *section, continue; } + /* skip unnecessary parsing */ + if ( (parse_options && !section->is_options) + || (!parse_options && section->is_options) ) { + continue; + } + /* directive */ /* strsep modifies the 'line' string: 'key \0 value' */ key = line; -- 1.7.8.3
On Mon, Jan 9, 2012 at 12:09 PM, jjacky <i.am.jack.mail@gmail.com> wrote:
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them)
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com>
I think this will fail hard in the following scenario: pacman.conf follows: ----- Include /etc/pacman.d/shared-options.conf [repo] .... ----- Include directives can occur *anywhere*, not just inside sections.
--- src/pacman/conf.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 7ba2791..09749ea 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -755,6 +755,12 @@ static int _parseconfig(const char *file, struct section_t *section, continue; }
+ /* skip unnecessary parsing */ + if ( (parse_options && !section->is_options) + || (!parse_options && section->is_options) ) { + continue; + } + /* directive */ /* strsep modifies the 'line' string: 'key \0 value' */ key = line; -- 1.7.8.3
On 01/09/12 19:40, Dan McGee wrote:
On Mon, Jan 9, 2012 at 12:09 PM, jjacky<i.am.jack.mail@gmail.com> wrote:
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them)
Signed-off-by: Olivier Brunel<i.am.jack.mail@gmail.com>
I think this will fail hard in the following scenario:
pacman.conf follows: ----- Include /etc/pacman.d/shared-options.conf
[repo] .... -----
Include directives can occur *anywhere*, not just inside sections.
hmm... so you mean there could be Include directives even before the very first section? The way I read the man page was that Include directives, like everything else, are always in a section. In such cases, the Include directive would be in the "options" section so it would be parsed, but only when parsing for options, not when doing so for repos. And vice versa. It would fail, however, with Include directives before any section is open, so not in any section, yes. Oh, but yeah, it would also fail if an Include in the "options" section was to define some repos, or if an Include in a repo was to include a section "options" of its own, indeed. My bad.
--- src/pacman/conf.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 7ba2791..09749ea 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -755,6 +755,12 @@ static int _parseconfig(const char *file, struct section_t *section, continue; }
+ /* skip unnecessary parsing */ + if ( (parse_options&& !section->is_options) + || (!parse_options&& section->is_options) ) { + continue; + } + /* directive */ /* strsep modifies the 'line' string: 'key \0 value' */ key = line; -- 1.7.8.3
On 01/09/12 19:40, Dan McGee wrote:
On Mon, Jan 9, 2012 at 12:09 PM, jjacky <i.am.jack.mail@gmail.com> wrote:
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them)
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com>
I think this will fail hard in the following scenario:
pacman.conf follows: ----- Include /etc/pacman.d/shared-options.conf
[repo] .... -----
Include directives can occur *anywhere*, not just inside sections.
Sorry, I'm back on this (because I work on something that needs to parse pacman.conf) First off, actually your example was invalid, as there cannot be directives outside of any section, pacman would even throw a syntax error ("All directives must belong to a section"). What is true, however, is that inside an included file new sections can be opened. So as I said last time, if a file included in "options" was to define some repos, my patch would fail (since any & all Include directive in repos would be ignored during options pass, and vice versa). However, I feel like I should ask: is it really the expected behavior? That pacman allows/supports sections being defined inside included files? Because this could lead to trouble; for instance, w/ this: -- pacman.conf ------- [options] Include = /etc/pacman.d/shared.conf DBPath = /opt/pacman/ ---------------------- -- shared.conf ------- [core] Server = http//foo.bar/ ---------------------- Then by the time pacman gets to the DBPath directive, it thinks the current section is "core" and therefore will *not* apply that directive. That doesn't seem very right to me? In fact, this would result in a warning: "directive 'DBPath' in section 'core' not recognized." One could also imagine that servers get added to the wrong repo with the same kind of situation. Only then, there would be no warnings! Is this really the expected behavior? A fix for this could be to use a new struct section_t when parsing an included file, so as to not "mess up" the current one. (And my patch would still be invalid, for the same reason it is now.) Or, to ignore sections in included file, and going with the idea that all directives in an included file belong to the section where the Include directive was in. (In which case I think my patch could work, since e.g. during the options pass there would be no need to process Include directives in repos, and vice versa.) I also feel the later might make things clearer, but I'm sure there might be usage cases I'm not thinking of, where it might be good to define sections inside included files? Any ideas/thoughts on this? -jjacky
--- src/pacman/conf.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 7ba2791..09749ea 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -755,6 +755,12 @@ static int _parseconfig(const char *file, struct section_t *section, continue; }
+ /* skip unnecessary parsing */ + if ( (parse_options && !section->is_options) + || (!parse_options && section->is_options) ) { + continue; + } + /* directive */ /* strsep modifies the 'line' string: 'key \0 value' */ key = line; -- 1.7.8.3
On Tue, Jan 10, 2012 at 7:37 AM, jjacky <i.am.jack.mail@gmail.com> wrote:
On 01/09/12 19:40, Dan McGee wrote:
On Mon, Jan 9, 2012 at 12:09 PM, jjacky <i.am.jack.mail@gmail.com> wrote:
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them)
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com>
I think this will fail hard in the following scenario:
pacman.conf follows: ----- Include /etc/pacman.d/shared-options.conf
[repo] .... -----
Include directives can occur *anywhere*, not just inside sections.
Sorry, I'm back on this (because I work on something that needs to parse pacman.conf)
First off, actually your example was invalid, as there cannot be directives outside of any section, pacman would even throw a syntax error ("All directives must belong to a section").
Ahh, tested and confirmed, duh. Thanks for looking at this.
What is true, however, is that inside an included file new sections can be opened. So as I said last time, if a file included in "options" was to define some repos, my patch would fail (since any & all Include directive in repos would be ignored during options pass, and vice versa). Correct.
However, I feel like I should ask: is it really the expected behavior? That pacman allows/supports sections being defined inside included files? 100% correct that you can *define and use them* in an include file, yes. In fact, I'm thinking the first "all directives" check should not be so strict here, if the first line of an Included file is a [section] header, we should be allowing that.
Because this could lead to trouble; for instance, w/ this:
-- pacman.conf ------- [options] Include = /etc/pacman.d/shared.conf DBPath = /opt/pacman/ ----------------------
-- shared.conf ------- [core] Server = http//foo.bar/ ----------------------
Then by the time pacman gets to the DBPath directive, it thinks the current section is "core" and therefore will *not* apply that directive. That doesn't seem very right to me?
In fact, this would result in a warning: "directive 'DBPath' in section 'core' not recognized."
One could also imagine that servers get added to the wrong repo with the same kind of situation. Only then, there would be no warnings!
Is this really the expected behavior?
A fix for this could be to use a new struct section_t when parsing an included file, so as to not "mess up" the current one. (And my patch would still be invalid, for the same reason it is now.) This is my vote, as I agree, the current parsing is not so sane when returning back from a file. We should probably only allow sections to
Say a sysadmin wanted to change repos on 15 machines but couldn't share the pacman.conf file across them; it would be much easier if all repos were defined in a separate include file and that file could be swapped on all the machines. propagate down the include chain and not back up, as you have indicated, so this seems like an acceptable resolution to me.
Or, to ignore sections in included file, and going with the idea that all directives in an included file belong to the section where the Include directive was in. (In which case I think my patch could work, since e.g. during the options pass there would be no need to process Include directives in repos, and vice versa.)
I also feel the later might make things clearer, but I'm sure there might be usage cases I'm not thinking of, where it might be good to define sections inside included files?
Any ideas/thoughts on this? Can you explain the original rationale behind your patch- was it just to save some cycles? The config parsing is extremely fast and that is why I had no qualms switching it to doing it twice.
-Dan
On 01/10/12 15:25, Dan McGee wrote:
On Tue, Jan 10, 2012 at 7:37 AM, jjacky <i.am.jack.mail@gmail.com> wrote:
On 01/09/12 19:40, Dan McGee wrote:
On Mon, Jan 9, 2012 at 12:09 PM, jjacky <i.am.jack.mail@gmail.com> wrote:
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them)
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com>
I think this will fail hard in the following scenario:
pacman.conf follows: ----- Include /etc/pacman.d/shared-options.conf
[repo] .... -----
Include directives can occur *anywhere*, not just inside sections.
Sorry, I'm back on this (because I work on something that needs to parse pacman.conf)
First off, actually your example was invalid, as there cannot be directives outside of any section, pacman would even throw a syntax error ("All directives must belong to a section").
Ahh, tested and confirmed, duh. Thanks for looking at this.
What is true, however, is that inside an included file new sections can be opened. So as I said last time, if a file included in "options" was to define some repos, my patch would fail (since any & all Include directive in repos would be ignored during options pass, and vice versa). Correct.
However, I feel like I should ask: is it really the expected behavior? That pacman allows/supports sections being defined inside included files? 100% correct that you can *define and use them* in an include file, yes. In fact, I'm thinking the first "all directives" check should not be so strict here, if the first line of an Included file is a [section] header, we should be allowing that.
Say a sysadmin wanted to change repos on 15 machines but couldn't share the pacman.conf file across them; it would be much easier if all repos were defined in a separate include file and that file could be swapped on all the machines.
Because this could lead to trouble; for instance, w/ this:
-- pacman.conf ------- [options] Include = /etc/pacman.d/shared.conf DBPath = /opt/pacman/ ----------------------
-- shared.conf ------- [core] Server = http//foo.bar/ ----------------------
Then by the time pacman gets to the DBPath directive, it thinks the current section is "core" and therefore will *not* apply that directive. That doesn't seem very right to me?
In fact, this would result in a warning: "directive 'DBPath' in section 'core' not recognized."
One could also imagine that servers get added to the wrong repo with the same kind of situation. Only then, there would be no warnings!
Is this really the expected behavior?
A fix for this could be to use a new struct section_t when parsing an included file, so as to not "mess up" the current one. (And my patch would still be invalid, for the same reason it is now.) This is my vote, as I agree, the current parsing is not so sane when returning back from a file. We should probably only allow sections to propagate down the include chain and not back up, as you have indicated, so this seems like an acceptable resolution to me.
Alright; So I'll look into this and see to send a patch to do that.
Or, to ignore sections in included file, and going with the idea that all directives in an included file belong to the section where the Include directive was in. (In which case I think my patch could work, since e.g. during the options pass there would be no need to process Include directives in repos, and vice versa.)
I also feel the later might make things clearer, but I'm sure there might be usage cases I'm not thinking of, where it might be good to define sections inside included files?
Any ideas/thoughts on this? Can you explain the original rationale behind your patch- was it just to save some cycles? The config parsing is extremely fast and that is why I had no qualms switching it to doing it twice.
Yes, just to save cycles. It's simply because I was looking at how this work, and noticed that included files were read twice (one per pass) while I thought only one time was needed, so I figured I might save a few "useless" stuff, is all. Never had any problems or anything feel slow or anything. -j
-Dan
On 01/10/12 15:25, Dan McGee wrote:
On Tue, Jan 10, 2012 at 7:37 AM, jjacky <i.am.jack.mail@gmail.com> wrote:
On 01/09/12 19:40, Dan McGee wrote:
On Mon, Jan 9, 2012 at 12:09 PM, jjacky <i.am.jack.mail@gmail.com> wrote:
parsing config file is a two-steps process, one for "options" and one for the repos. in each of those, there's no need to parse the other section(s), so we'll skip it. (most notably, when parsing for "options" it would read all included files for repos, while just ignoring everything in them)
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com>
I think this will fail hard in the following scenario:
pacman.conf follows: ----- Include /etc/pacman.d/shared-options.conf
[repo] .... -----
Include directives can occur *anywhere*, not just inside sections.
Sorry, I'm back on this (because I work on something that needs to parse pacman.conf)
First off, actually your example was invalid, as there cannot be directives outside of any section, pacman would even throw a syntax error ("All directives must belong to a section").
Ahh, tested and confirmed, duh. Thanks for looking at this.
What is true, however, is that inside an included file new sections can be opened. So as I said last time, if a file included in "options" was to define some repos, my patch would fail (since any & all Include directive in repos would be ignored during options pass, and vice versa). Correct.
However, I feel like I should ask: is it really the expected behavior? That pacman allows/supports sections being defined inside included files? 100% correct that you can *define and use them* in an include file, yes. In fact, I'm thinking the first "all directives" check should not be so strict here, if the first line of an Included file is a [section] header, we should be allowing that.
Say a sysadmin wanted to change repos on 15 machines but couldn't share the pacman.conf file across them; it would be much easier if all repos were defined in a separate include file and that file could be swapped on all the machines.
Because this could lead to trouble; for instance, w/ this:
-- pacman.conf ------- [options] Include = /etc/pacman.d/shared.conf DBPath = /opt/pacman/ ----------------------
-- shared.conf ------- [core] Server = http//foo.bar/ ----------------------
Then by the time pacman gets to the DBPath directive, it thinks the current section is "core" and therefore will *not* apply that directive. That doesn't seem very right to me?
In fact, this would result in a warning: "directive 'DBPath' in section 'core' not recognized."
One could also imagine that servers get added to the wrong repo with the same kind of situation. Only then, there would be no warnings!
Is this really the expected behavior?
A fix for this could be to use a new struct section_t when parsing an included file, so as to not "mess up" the current one. (And my patch would still be invalid, for the same reason it is now.) This is my vote, as I agree, the current parsing is not so sane when returning back from a file. We should probably only allow sections to propagate down the include chain and not back up, as you have indicated, so this seems like an acceptable resolution to me.
Alright, so looking at this, it seems a bit more complicated than I thought. Because I think that pacman currently expects to only ever process any section just once. That is, a call to finish_section() should only be made once per repo, to register the db. But if I create a new section_t when processing an include file, and said file contains multiple repos (or just another section really), if one of those is also featured on pacman.conf then there would be a second call to finish_section() with the same repo name, and that would fail. In fact this is not linked to Include directives, would be the same with something like this: -- pacman.conf ---------------- [core] Server = http://foo/ [core] Server = http://bar/ ------------------------------- (Obviously one probably wouldn't do that in one file, but through Includes it could happen and be less obvious...) Did a very quick test w/ such a file, and it seems pacman is actually fine with this, only it will register 2 dbs by the same name, which I'm guessing might only lead to troubles, no? Which mean that in finish_section() we should first check if there's already a db registered by that name, if not create it (else get it), and then simply add servers to it. And also, after having parsed an Include, and if it is in the same section, we shall "import" servers into the current section so they get processed on this section's finish_section() call. And if it's not the same section, just call finish_section(). Would this be correct? -jacky
Or, to ignore sections in included file, and going with the idea that all directives in an included file belong to the section where the Include directive was in. (In which case I think my patch could work, since e.g. during the options pass there would be no need to process Include directives in repos, and vice versa.)
I also feel the later might make things clearer, but I'm sure there might be usage cases I'm not thinking of, where it might be good to define sections inside included files?
Any ideas/thoughts on this? Can you explain the original rationale behind your patch- was it just to save some cycles? The config parsing is extremely fast and that is why I had no qualms switching it to doing it twice.
-Dan
When parsing Include directives, a new section_t is now used, to ensure it won't "mess" with the current one (since included files can define new sections, which would result in an invalid "change" of the current section) This new section starts "in" the current one (so included files are parsed as expected, and don't need to open any section); IOW sections propagate down the include chain, not back up. This also solves the bug (?) when two repos with the same name are declared, which could happen in the same file or, more likely, through Include-s. Now only one db will be registered, with all servers in it (it used to be as many dbs with the same name as there were sections). Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com> --- src/pacman/conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 2ff16d2..eb3374e 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -628,6 +628,18 @@ struct section_t { alpm_list_t *servers; }; +static alpm_db_t *get_db(const char *dbname) +{ + alpm_list_t *i; + for(i = alpm_option_get_syncdbs(config->handle); i; i = i->next) { + alpm_db_t *db = i->data; + if(strcmp(alpm_db_get_name(db), dbname) == 0) { + return db; + } + } + return NULL; +} + /** * Wrap up a section once we have reached the end of it. This should be called * when a subsequent section is encountered, or when we have reached the end of @@ -650,13 +662,17 @@ static int finish_section(struct section_t *section, int parse_options) goto cleanup; } - /* if we are not looking at options sections only, register a db */ - db = alpm_db_register_sync(config->handle, section->name, section->siglevel); + /* if we are not looking at options sections only, get the db */ + db = get_db(section->name); if(db == NULL) { - pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), - section->name, alpm_strerror(alpm_errno(config->handle))); - ret = 1; - goto cleanup; + /* not registered yet, let's do it */ + db = alpm_db_register_sync(config->handle, section->name, section->siglevel); + if(db == NULL) { + pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), + section->name, alpm_strerror(alpm_errno(config->handle))); + ret = 1; + goto cleanup; + } } for(i = section->servers; i; i = alpm_list_next(i)) { @@ -780,6 +796,7 @@ static int _parseconfig(const char *file, struct section_t *section, glob_t globbuf; int globret; size_t gindex; + struct section_t inc_section; if(value == NULL) { pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), @@ -816,10 +833,33 @@ static int _parseconfig(const char *file, struct section_t *section, file, linenum, value); break; default: + /* create new section_t for the includes -- to keep ours unaffected */ for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { + /* we use a different section_t for includes, so they can't mess up + * the current one. This section starts "in" the current section ofc */ + inc_section->name = strdup (section->name); + inc_section->is_options = section->is_options; + inc_section->siglevel = section->siglevel; + inc_section->servers = NULL; pm_printf(ALPM_LOG_DEBUG, "config file %s, line %d: including %s\n", file, linenum, globbuf.gl_pathv[gindex]); - _parseconfig(globbuf.gl_pathv[gindex], section, parse_options, depth + 1); + _parseconfig(globbuf.gl_pathv[gindex], inc_section, parse_options, depth + 1); + /* now, if this is a repo ... */ + if(!parse_options && !section->is_options) { + if (section->siglevel == inc_section->siglevel + && strcmp(section->name, inc_section->name) == 0) { + /* ... still the same one, we'll just import servers */ + alpm_list_t *i; + for(i = inc_section->servers; i; i = alpm_list_next(i)) { + section->servers = alpm_list_add(section->servers, i->data); + } + alpm_list_free(inc_section->servers); + free(inc_section->name); + } else { + /* ... a different one, so let's "finish" it */ + finish_section(inc_section, parse_options); + } + } } break; } -- 1.7.8.3
On Wed, Jan 11, 2012 at 12:52 PM, Olivier Brunel <i.am.jack.mail@gmail.com> wrote:
When parsing Include directives, a new section_t is now used, to ensure it won't "mess" with the current one (since included files can define new sections, which would result in an invalid "change" of the current section) This new section starts "in" the current one (so included files are parsed as expected, and don't need to open any section); IOW sections propagate down the include chain, not back up.
This also solves the bug (?) when two repos with the same name are declared, which could happen in the same file or, more likely, through Include-s. Now only one db will be registered, with all servers in it (it used to be as many dbs with the same name as there were sections).
IOW? bug (?)? "quotes" around "random" words? I know I'm nitpicking, but this is not always the most readable English, especially to non-native speakers (even I had to look up what IOW was). Imagine coming back to a commit message in two years- if there is anything in it that wouldn't make sense, you need to reword/rewrite it. That is the purpose of commit messages.
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com> --- src/pacman/conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 47 insertions(+), 7 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 2ff16d2..eb3374e 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -628,6 +628,18 @@ struct section_t { alpm_list_t *servers; };
+static alpm_db_t *get_db(const char *dbname) +{ + alpm_list_t *i; + for(i = alpm_option_get_syncdbs(config->handle); i; i = i->next) { + alpm_db_t *db = i->data; + if(strcmp(alpm_db_get_name(db), dbname) == 0) { + return db; + } + } + return NULL; +} Rather than copying this straight out of src/pacman/sync.c, why don't you make it visible to the entire compilation unit in util.c/util.h (this file) and remove the now unneeded version from sync.c? (for a more public function, a longer name might be nice too; something like 'get_sync_db' just to be as clear as possible).
+ /** * Wrap up a section once we have reached the end of it. This should be called * when a subsequent section is encountered, or when we have reached the end of @@ -650,13 +662,17 @@ static int finish_section(struct section_t *section, int parse_options) goto cleanup; }
- /* if we are not looking at options sections only, register a db */ - db = alpm_db_register_sync(config->handle, section->name, section->siglevel); + /* if we are not looking at options sections only, get the db */ + db = get_db(section->name); if(db == NULL) { - pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), - section->name, alpm_strerror(alpm_errno(config->handle))); - ret = 1; - goto cleanup; + /* not registered yet, let's do it */ The "let's do it" part is not really necessary; I say this because we also try to avoid contractions if possible.
+ db = alpm_db_register_sync(config->handle, section->name, section->siglevel); + if(db == NULL) { + pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), + section->name, alpm_strerror(alpm_errno(config->handle))); + ret = 1; + goto cleanup; + } }
for(i = section->servers; i; i = alpm_list_next(i)) { @@ -780,6 +796,7 @@ static int _parseconfig(const char *file, struct section_t *section, glob_t globbuf; int globret; size_t gindex; + struct section_t inc_section;
if(value == NULL) { pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), @@ -816,10 +833,33 @@ static int _parseconfig(const char *file, struct section_t *section, file, linenum, value); break; default: + /* create new section_t for the includes -- to keep ours unaffected */ for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { + /* we use a different section_t for includes, so they can't mess up + * the current one. This section starts "in" the current section ofc */ ofc?
+ inc_section->name = strdup (section->name); + inc_section->is_options = section->is_options; + inc_section->siglevel = section->siglevel; + inc_section->servers = NULL; pm_printf(ALPM_LOG_DEBUG, "config file %s, line %d: including %s\n", file, linenum, globbuf.gl_pathv[gindex]); - _parseconfig(globbuf.gl_pathv[gindex], section, parse_options, depth + 1); + _parseconfig(globbuf.gl_pathv[gindex], inc_section, parse_options, depth + 1); + /* now, if this is a repo ... */ + if(!parse_options && !section->is_options) { + if (section->siglevel == inc_section->siglevel + && strcmp(section->name, inc_section->name) == 0) { This logic seems very very suspect to me; and likely wrong. For example:
[myrepo] SigLevel = DatabaseRequired PackageOptional Server = foobar.com Include = /path/to/myrepo.override ----> (jump to new file) Server = example.com SigLevel = PackageRequred ----> EOF, jump back You're going to lose my updated siglevel configuration in all of this; I'm not even sure what is going to happen. I also don't like the assumption that repos have only "Server" as their main setting, I have a few plans to add more options to this in the future and don't want parsing to rely on the fact that right now only "Server" and "SigLevel" appear there, if possible.
+ /* ... still the same one, we'll just import servers */ + alpm_list_t *i; + for(i = inc_section->servers; i; i = alpm_list_next(i)) { + section->servers = alpm_list_add(section->servers, i->data); + } + alpm_list_free(inc_section->servers); + free(inc_section->name); + } else { + /* ... a different one, so let's "finish" it */ + finish_section(inc_section, parse_options); + } + } } break; } -- 1.7.8.3
On 01/11/12 23:00, Dan McGee wrote:
On Wed, Jan 11, 2012 at 12:52 PM, Olivier Brunel <i.am.jack.mail@gmail.com> wrote:
When parsing Include directives, a new section_t is now used, to ensure it won't "mess" with the current one (since included files can define new sections, which would result in an invalid "change" of the current section) This new section starts "in" the current one (so included files are parsed as expected, and don't need to open any section); IOW sections propagate down the include chain, not back up.
This also solves the bug (?) when two repos with the same name are declared, which could happen in the same file or, more likely, through Include-s. Now only one db will be registered, with all servers in it (it used to be as many dbs with the same name as there were sections).
IOW? bug (?)? "quotes" around "random" words? I know I'm nitpicking, but this is not always the most readable English, especially to non-native speakers (even I had to look up what IOW was).
Imagine coming back to a commit message in two years- if there is anything in it that wouldn't make sense, you need to reword/rewrite it. That is the purpose of commit messages.
Noted, sorry. (To be honest I am very new at all this, so I probably lack proper habits/methodologies. I'll do my best to improve this.)
Signed-off-by: Olivier Brunel <i.am.jack.mail@gmail.com> --- src/pacman/conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 47 insertions(+), 7 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 2ff16d2..eb3374e 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -628,6 +628,18 @@ struct section_t { alpm_list_t *servers; };
+static alpm_db_t *get_db(const char *dbname) +{ + alpm_list_t *i; + for(i = alpm_option_get_syncdbs(config->handle); i; i = i->next) { + alpm_db_t *db = i->data; + if(strcmp(alpm_db_get_name(db), dbname) == 0) { + return db; + } + } + return NULL; +} Rather than copying this straight out of src/pacman/sync.c, why don't you make it visible to the entire compilation unit in util.c/util.h (this file) and remove the now unneeded version from sync.c? (for a more public function, a longer name might be nice too; something like 'get_sync_db' just to be as clear as possible).
+ /** * Wrap up a section once we have reached the end of it. This should be called * when a subsequent section is encountered, or when we have reached the end of @@ -650,13 +662,17 @@ static int finish_section(struct section_t *section, int parse_options) goto cleanup; }
- /* if we are not looking at options sections only, register a db */ - db = alpm_db_register_sync(config->handle, section->name, section->siglevel); + /* if we are not looking at options sections only, get the db */ + db = get_db(section->name); if(db == NULL) { - pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), - section->name, alpm_strerror(alpm_errno(config->handle))); - ret = 1; - goto cleanup; + /* not registered yet, let's do it */ The "let's do it" part is not really necessary; I say this because we also try to avoid contractions if possible.
+ db = alpm_db_register_sync(config->handle, section->name, section->siglevel); + if(db == NULL) { + pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), + section->name, alpm_strerror(alpm_errno(config->handle))); + ret = 1; + goto cleanup; + } }
for(i = section->servers; i; i = alpm_list_next(i)) { @@ -780,6 +796,7 @@ static int _parseconfig(const char *file, struct section_t *section, glob_t globbuf; int globret; size_t gindex; + struct section_t inc_section;
if(value == NULL) { pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), @@ -816,10 +833,33 @@ static int _parseconfig(const char *file, struct section_t *section, file, linenum, value); break; default: + /* create new section_t for the includes -- to keep ours unaffected */ for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { + /* we use a different section_t for includes, so they can't mess up + * the current one. This section starts "in" the current section ofc */ ofc?
+ inc_section->name = strdup (section->name); + inc_section->is_options = section->is_options; + inc_section->siglevel = section->siglevel; + inc_section->servers = NULL; pm_printf(ALPM_LOG_DEBUG, "config file %s, line %d: including %s\n", file, linenum, globbuf.gl_pathv[gindex]); - _parseconfig(globbuf.gl_pathv[gindex], section, parse_options, depth + 1); + _parseconfig(globbuf.gl_pathv[gindex], inc_section, parse_options, depth + 1); + /* now, if this is a repo ... */ + if(!parse_options && !section->is_options) { + if (section->siglevel == inc_section->siglevel + && strcmp(section->name, inc_section->name) == 0) { This logic seems very very suspect to me; and likely wrong. For example:
[myrepo] SigLevel = DatabaseRequired PackageOptional Server = foobar.com Include = /path/to/myrepo.override ----> (jump to new file) Server = example.com SigLevel = PackageRequred ----> EOF, jump back
You're going to lose my updated siglevel configuration in all of this; I'm not even sure what is going to happen.
I also don't like the assumption that repos have only "Server" as their main setting, I have a few plans to add more options to this in the future and don't want parsing to rely on the fact that right now only "Server" and "SigLevel" appear there, if possible.
Right. This was to go with the idea that changes should only go down, not up. Right now since the same section_t is used, they go both ways indeed, but that can lead to troubles when defining sections in included files. Thing is, supporting it (sections in included files) means that you could have e.g. this: [myrepo] SigLevel = DatabaseRequired PackageOptional Server = foobar.com Include = /path/to/myrepo.override ----> (jump to new file) [anotherrepo] Server = foo.com [myrepo] Server = example.com SigLevel = PackageRequred ----> EOF, jump back With the patch, the updated siglevel would indeed not be applied. Currently, pacman would register two dbs by the name "myrepo" -- is that how it is supposed to work? Or should everything be remembered until all files have been parsed, and only then dbs be registered? -j
+ /* ... still the same one, we'll just import servers */ + alpm_list_t *i; + for(i = inc_section->servers; i; i = alpm_list_next(i)) { + section->servers = alpm_list_add(section->servers, i->data); + } + alpm_list_free(inc_section->servers); + free(inc_section->name); + } else { + /* ... a different one, so let's "finish" it */ + finish_section(inc_section, parse_options); + } + } } break; } -- 1.7.8.3
participants (3)
-
Dan McGee
-
jjacky
-
Olivier Brunel