Skip to content

add support for struct entities in MakeVariant#329

Closed
cdoern wants to merge 1 commit intogodbus:masterfrom
cdoern:array
Closed

add support for struct entities in MakeVariant#329
cdoern wants to merge 1 commit intogodbus:masterfrom
cdoern:array

Conversation

@cdoern
Copy link
Contributor

@cdoern cdoern commented Jul 6, 2022

currently, structs are not parsed properly since format() has no handling for the reflect type.
add a switch case to handle structs and encode some sane defaults if given an empty one

resolves #328

Signed-off-by: Charlie Doern [email protected]

@giuseppe
Copy link
Contributor

giuseppe commented Jul 6, 2022

@guelfey PTAL

@guelfey
Copy link
Member

guelfey commented Jul 6, 2022

Generally makes sense, however I'd like to stick to the GVariant format from GLib since this seems to be the convention for other tools, and DBus structs are called "tuples" there: https://www.freedesktop.org/software/gstreamer-sdk/data/docs/2012.5/glib/gvariant-text.html#gvariant-text-tuples ar the best docs I can find. So normal parantheses and separating the field with a comma.

@cdoern
Copy link
Contributor Author

cdoern commented Jul 6, 2022

@guelfey ok thanks for the feedback. I wasn't sure what the format should be so I'll repush with those changes tonight.

@cdoern
Copy link
Contributor Author

cdoern commented Jul 7, 2022

@giuseppe, using this PR in c/common the unit file generated looks like

# This is a transient unit file, created programmatically via the systemd API. Do not edit.
[Unit]
Description=cgroup machine2-libpod_pod_105.slice
Wants=machine2.slice
DefaultDependencies=no

[Slice]
MemoryAccounting=yes
CPUAccounting=yes
IOAccounting=yes
CPUWeight=4
MemoryMax=900
MemorySwapMax=1000
CPUQuotaPeriodSec=100ms
CPUQuota=100%
AllowedCPUs=4-5
AllowedMemoryNodes=4-5
IOReadBandwidthMax=
IOReadBandwidthMax=8:16 2097152

which is incorrectly adding an extra blank line, nothing is being added to io.max. The final property being provided is {IOReadBandwidthMax [("8:16", 2097152)]} does this seem right to you? not sure where to go from here.

@giuseppe
Copy link
Contributor

giuseppe commented Jul 7, 2022

how does the generated value you pass to systemd look like? Not sure if the empty io.max depends on that extra empty line

@cdoern
Copy link
Contributor Author

cdoern commented Jul 7, 2022

@giuseppe running something like:

...
for k, v := range structMap {
			vari := dbus.MakeVariant(v)
			fmt.Println(vari.Signature(), vari.String(), vari.Value())
...

prints:

a(st) @a(st) [("8:16" 2097152)] [{8:16 2097152}]

as a matter of fact, the extra lines are added to the unit file even without these modifications...

@cdoern
Copy link
Contributor Author

cdoern commented Jul 7, 2022

@guelfey this should be good to go! works with my PR in containers/common

currently, structs are not parsed properly since `format()` has no handling for the reflect type
add a switch case to handle structs and encode some sane defaults if given an empty one

resolves godbus#328

Signed-off-by: Charlie Doern <[email protected]>
@cdoern
Copy link
Contributor Author

cdoern commented Aug 11, 2022

@guelfey sorry I took so long here. To answer your question about why this is needed, mainly for testing and verification that systemd is getting the right values before we merge something into containers/common. Otherwise, debug statements just print INVALID

Copy link
Member

@guelfey guelfey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

@guelfey guelfey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschmied
Copy link

Hey, @cdoern -- is this going anywhere? I am interested in the fork of routerd/go-firewalld but it seems that work is blocked on merging this? @grmrgecko ?

@grmrgecko
Copy link

@rschmied I don't think it blocks my PR, but there are 4 methods I implemented that will not work until this is done. Upstream of go-firewalld hasn't responded about my changes, but you can use it with the following added to the go module file.

replace github.com/routerd/go-firewalld => github.com/grmrgecko/go-firewalld

If routerd doesn't respond, I wouldn't mind changing the module file in my fork to allow people to just use it without the replace declaration.

@rschmied
Copy link

rschmied commented Mar 5, 2025

Thanks for this. I'd be fine with using your fork but then there's still the additional dependency on this, right? E.g. I'd have to use two unmerged PRs / different forks to get the whole functionality (the additional 4 methods plus the required dbus support) , right?

@cdoern cdoern closed this Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MakeVariant cannot properly evaluate a(st)

5 participants