[pacman-dev] [PATCH 1/2] skip unneeded parsing

[jjacky]

On 01/10/12 15:25, Dan McGee wrote:
> On Tue, Jan 10, 2012 at 7:37 AM, jjacky <i.am.jack.mail at 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 at 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 at 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
>