feat: initial work on supporting dbus-daemon XML configuration files #146

jokeyrhyme · 2024-10-06T11:33:35Z

Howdie

I figured I knew just enough Rust to work on this whilst learning how to handle XML in Rust, and thought it'd be just tedious enough that nobody else had started this

Then I realised someone has indeed started this over in #23 , haha!

Anyhow, this uses quick-xml, too, but this PR version models just a little bit more in serde, so it ends up being roughly equivalent with ~20% fewer lines of code (but probably takes slightly longer to compile), but the other PR does do a bit more modeling of what's possible in the <listen /> elements

I do have an integration test in this PR that runs the XML deserializer against all the real XML configuration files that are found, and it does pass on my system which is promising (although the hard-coded test cases in the other PR are probably the better bet in the long term)

You probably won't end up using this, but perhaps if the other folks take a look there's something salvageable from this attempt

If you do like the look of this, just let me know what blockers I need to fix up to get it into a merge-worthy state

<3

src/configuration.rs

zeenix · 2024-10-07T11:18:18Z

Anyhow, this uses quick-xml, too, but this PR version models just a little bit more in serde, so it ends up being roughly equivalent with ~20% fewer lines of code (but probably takes slightly longer to compile), but the other PR does do a bit more modeling of what's possible in the <listen /> elements

We probably want to mix combine the best of both. In terms of tests, I like the approach in #23, so the tests are very standalone and cross-platform even. @elmarco any chance, you can take parts from this PR to yours when you rebase and revive it?

@jokeyrhyme btw in anticipation of future contributions from you, I'd like to refer to you our contribution guidelines. The emojis are optional for flyby commits but please keep the commits atomic (in the sense that they bring one specific logical change, regardless of the size and when you wrote the code). #23 was done before we started using emoji prefixes in commits but atomic commits rule is something me and @elmarco have followed for almost 2 decades now. :)

zeenix · 2024-10-07T11:20:51Z

src/configuration/raw.rs

+
+#[derive(Clone, Debug, Default, Deserialize, PartialEq)]
+#[serde(default)]
+pub(super) struct RawConfiguration {


It's under raw mod, so Raw prefixes in the names are unnecessary. If your'e coming from a C background, I know from my personal experience, that this is one habit that's hard to get rid of. :)

Some more stuff:

Instead of pub(super) everywhere, you can just have them pub` and then from the supermodule, you keep the mod private and that achieves the same.

It would be good to have some docs for each item ideally but one thing that definitely need some docs is what the raw API is all about (through a module-level documentation comment).

zeenix · 2024-10-07T11:32:01Z

It would be good to have some docs for each item ideally but one thing that definitely need some docs is what the raw API is all about (through a module-level documentation comment).

I'm actually confused by this raw separation. I think we should only need 1 representation and (de)serialization should make it happen. One can always manually impl serde::{Serialize, Deserialize} where needed.

elmarco · 2024-10-07T11:37:38Z

@zeenix & @jokeyrhyme I plan to review this MR when I get to rebase #23. Thanks for the work!

zeenix · 2024-10-07T16:40:03Z

@zeenix & @jokeyrhyme I plan to review this MR when I get to rebase #23. Thanks for the work!

Great, thanks!

Btw, please keep in mind the description of the policy issue (#79) here. We should ignore (perhaps with a warning) any xml fragments that we don't want to implement and not create any types/code for parsing those bits. Apart from the example in #79, we should also not bother with limit elements. We should just hardcode their values in the code. I've not even known anyone to modify those values in config.

On the bright side, this also makes this task easier.

zeenix · 2024-10-07T17:11:27Z

Apart from the example in #79, we should also not bother with limit elements. We should just hardcode their values in the code. I've not even known anyone to modify those values in config.

I created #148 for these.

jokeyrhyme · 2024-10-07T20:00:26Z

I'm actually confused by this raw separation.

This is my first time playing with XML and serde, and it seemed necessary to have extra structs in a few places: https://docs.rs/quick-xml/latest/quick_xml/de/index.html#element-lists

So, the Raw stuff had these extra structs in place to guide serde as necessary, and then the main Configuration was a more convenient abstraction for the server code to work with

But yeah, I guess a custom Deserializer would be a more idiomatic way to implement this

zeenix · 2024-10-07T21:03:04Z

@jokeyrhyme Thanks for explaining.

But yeah, I guess a custom Deserializer would be a more idiomatic way to implement this

Right. Also as the docs you pointed to say, you might be able to make use of serde-query.

jokeyrhyme · 2024-10-12T06:55:30Z

I did mean to revisit this PR this weekend but I've been on baby duty :)

I'll carefully review your contribution guidelines, and maybe start a new PR that starts with git cherry-pick-ing XML work from the other PR

If @zeenix and @elmarco are still okay with me taking this on

jokeyrhyme · 2024-10-12T06:59:44Z

Regarding features we don't want, I also noticed that dbus-broker will warn that it doesn't implement eavesdrop policies, so maybe one planning/research exercise could be to run every option described in the dbus-daemon documentation through dbus-broker to see such warnings (or just read its code)

I'm guessing we'd probably want the parser to return these warnings as data rather than eprintln!() them out directly?

jokeyrhyme · 2024-10-12T07:08:52Z

https://github.com/search?q=repo%3Abus1%2Fdbus-broker%20deprecated&type=code

                if (cnode->type == CONFIG_NODE_DENY && cnode->allow_deny.send_requested_reply == UTIL_TRISTATE_YES)
                        fprintf(stderr, "Policy to deny expected replies in %s +%lu: Explicit policies on replies and errors are deprecated and ignored\n",
                                cnode->file, cnode->lineno);
                else if (cnode->type == CONFIG_NODE_ALLOW && cnode->allow_deny.send_requested_reply == UTIL_TRISTATE_NO)
                        fprintf(stderr, "Policy to allow unexpected replies in %s +%lu: Explicit policies on replies and errors are deprecated and ignored\n",
                                cnode->file, cnode->lineno);

        if (cnode->allow_deny.eavesdrop == UTIL_TRISTATE_YES) {
                if (cnode->type == CONFIG_NODE_ALLOW)
                        /* Ignore the attribute, but keep the rule, it also applies when not eavesdropping. */
                        fprintf(stderr, "Policy to allow eavesdropping in %s +%lu: Eavesdropping is deprecated and ignored\n",
                                cnode->file, cnode->lineno);
                else if (cnode->type == CONFIG_NODE_DENY)
                        /* The rule applies only when eavesdropping, drop it. */
                        return 0;
        }

                if (cnode->type == CONFIG_NODE_DENY && cnode->allow_deny.recv_requested_reply == UTIL_TRISTATE_YES)
                        fprintf(stderr, "Policy to deny expected replies in %s +%lu: Explicit policies on replies and errors are deprecated and ignored\n",
                                cnode->file, cnode->lineno);
                else if (cnode->type == CONFIG_NODE_ALLOW && cnode->allow_deny.recv_requested_reply == UTIL_TRISTATE_NO)
                        fprintf(stderr, "Policy to allow unexpected replies in %s +%lu: Explicit policies on replies and errors are deprecated and ignored\n",
                                cnode->file, cnode->lineno);

        if (cnode->allow_deny.eavesdrop == UTIL_TRISTATE_YES) {
                if (cnode->type == CONFIG_NODE_ALLOW)
                        /* Ignore the attribute, but keep the rule, it also applies when not eavesdropping. */
                        fprintf(stderr, "Policy to allow eavesdropping in %s +%lu: Eavesdropping is deprecated and ignored\n",
                                cnode->file, cnode->lineno);
                else if (cnode->type == CONFIG_NODE_DENY)
                        /* The rule applies only when eavesdropping, drop it. */
                        return 0;
        }

                if (i_cnode->type == CONFIG_NODE_POLICY &&
                    i_cnode->policy.context  == CONFIG_POLICY_AT_CONSOLE) {
                        fprintf(stderr, "Deprecated policy context in %s +%lu. The 'at_console' context is deprecated and will be ignored in the future.\n",
                                i_cnode->file, i_cnode->lineno);
                }

jokeyrhyme · 2024-10-13T00:08:10Z

Hmmm, my free time this weekend seems to have evaporated

I'm going to close this to signal clearly that I probably won't continue this in the foreseeable future, but if I get a good overlap of inspiration and free time soon I can always start that fresh branch, or even pick up some other feature that isn't already being tackled :)

zeenix · 2024-10-15T09:02:15Z

I can always start that fresh branch

Sure but then you can force to the same branch here and reuse the PR. Creating multiple PRs to achieve the same, only creates unnecessary noise in the GitHub records IMO.

So please do that, if you manage to get back to this. 🙏

jokeyrhyme · 2024-10-19T00:41:49Z

Hmmm, I've force-pushed to the same branch in my fork, and now the "Reopen and comment" button is disabled with "The branch was force-pushed or recreated" :S

zeenix · 2024-10-21T04:04:11Z

I've force-pushed to the same branch in my fork, and now the "Reopen and comment" button is disabled with "The branch was force-pushed or recreated" :S

Ughh.. It's probably because you closed the PR. The Reopen and comment is disabled button is disabled for me too now. It's ok, create a new PR this time but please don't close PRs in the future. I can do that when/if needed. :)

See: dbus2#146 (comment)

feat: initial work on supporting dbus-daemon XML configuration files

d6de3e4

jokeyrhyme mentioned this pull request Oct 6, 2024

WIP: run against dbus-broker bench #23

Closed

jokeyrhyme commented Oct 6, 2024

View reviewed changes

src/configuration.rs Outdated Show resolved Hide resolved

jokeyrhyme mentioned this pull request Oct 6, 2024

Ability to parse D-Bus configuration file(s) #78

Open

jokeyrhyme commented Oct 6, 2024

View reviewed changes

src/configuration.rs Outdated Show resolved Hide resolved

jokeyrhyme added 2 commits October 7, 2024 11:02

fix(xml): Raw... types internal-only, make other types pub

95a39b4

fix(xml): validate and convert RawRule... into useful Rule...

7adfdb5

jokeyrhyme force-pushed the main branch from 9878096 to 7adfdb5 Compare October 7, 2024 01:50

jokeyrhyme added 3 commits October 7, 2024 12:58

chore(xml): refactor Raw... out into separate file

ae79bf0

fix(xml): simplify PathBuf fields on Configuration

f88031f

chore(xml): extract Policy... and Rule... out into separate module

7ec0995

zeenix reviewed Oct 7, 2024

View reviewed changes

jokeyrhyme closed this Oct 13, 2024

jokeyrhyme mentioned this pull request Oct 27, 2024

parse XML configuration files #159

Open

41 tasks

jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 12, 2024

🔧 Ignore rule attributes ignored by dbus-broker

ce0eae5

See: dbus2#146 (comment)

jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 19, 2024

🔧 Ignore rule attributes ignored by dbus-broker

3a0bbca

See: dbus2#146 (comment)

jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 19, 2024

🔧 Ignore rule attributes ignored by dbus-broker

765b810

See: dbus2#146 (comment)

jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024

🔧 Ignore rule attributes ignored by dbus-broker

ee7494c

See: dbus2#146 (comment)

zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024

🔧 Ignore rule attributes ignored by dbus-broker

6060416

See: dbus2#146 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial work on supporting dbus-daemon XML configuration files #146

feat: initial work on supporting dbus-daemon XML configuration files #146

jokeyrhyme commented Oct 6, 2024 •

edited

Loading

zeenix commented Oct 7, 2024

zeenix Oct 7, 2024

zeenix Oct 7, 2024 •

edited

Loading

zeenix commented Oct 7, 2024

elmarco commented Oct 7, 2024

zeenix commented Oct 7, 2024

zeenix commented Oct 7, 2024

jokeyrhyme commented Oct 7, 2024 •

edited

Loading

zeenix commented Oct 7, 2024

jokeyrhyme commented Oct 12, 2024

jokeyrhyme commented Oct 12, 2024

jokeyrhyme commented Oct 12, 2024

jokeyrhyme commented Oct 13, 2024

zeenix commented Oct 15, 2024

jokeyrhyme commented Oct 19, 2024

zeenix commented Oct 21, 2024

feat: initial work on supporting dbus-daemon XML configuration files #146

feat: initial work on supporting dbus-daemon XML configuration files #146

Conversation

jokeyrhyme commented Oct 6, 2024 • edited Loading

zeenix commented Oct 7, 2024

zeenix Oct 7, 2024

Choose a reason for hiding this comment

zeenix Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

zeenix commented Oct 7, 2024

elmarco commented Oct 7, 2024

zeenix commented Oct 7, 2024

zeenix commented Oct 7, 2024

jokeyrhyme commented Oct 7, 2024 • edited Loading

zeenix commented Oct 7, 2024

jokeyrhyme commented Oct 12, 2024

jokeyrhyme commented Oct 12, 2024

jokeyrhyme commented Oct 12, 2024

jokeyrhyme commented Oct 13, 2024

zeenix commented Oct 15, 2024

jokeyrhyme commented Oct 19, 2024

zeenix commented Oct 21, 2024

jokeyrhyme commented Oct 6, 2024 •

edited

Loading

zeenix Oct 7, 2024 •

edited

Loading

jokeyrhyme commented Oct 7, 2024 •

edited

Loading