Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MakeVariant cannot properly evaluate a(st) #328

Open
cdoern opened this issue Jul 5, 2022 · 4 comments · May be fixed by #329
Open

MakeVariant cannot properly evaluate a(st) #328

cdoern opened this issue Jul 5, 2022 · 4 comments · May be fixed by #329

Comments

@cdoern
Copy link

cdoern commented Jul 5, 2022

in order to utilize the systemd BlockIOReadBandwidth cgroup entity, I need to be able to parse and pass an array of structs that each contain a string and a uint64 a(st). godbus is able to process the type via MakeVariant but the format() function in variant.go returns ["INVALID"] when given an a(st) type.

Am i doing something wrong here passing an array of structs or is there a gap in the parsing?

for example: giving a map of strings (a singular "BlockIOReadBandwidth") to structs map[string][]BlkioBpsThrottle that have the following format:

type BlkioBpsThrottle struct {
	Device   string
	Throttle uint64
}

and using the following loop to parse a map of length 1 with the device 8:16 and the throttle 2097152:

for k, v := range structMap {
			val := dbus.MakeVariant(v)
			fmt.Println(reflect.ValueOf(v), val.Signature(), val.Value(), val.String())
			p := systemdDbus.Property{
				Name:  k,
				Value: val,
			}

			properties = append(properties, p)

		}

results in: [{8:16 2097152}] a(st) [{8:16 2097152}] ["INVALID"]

the .String() function calls format() which has no handling for either a or (..). I have also noticed that MakeVariant seems to recursively call getSignature on a(st) even further confusing the output.

a(st) is a valid type according to: https://man7.org/linux/man-pages/man5/org.freedesktop.systemd1.5.html and looking at: https://dbus.freedesktop.org/doc/dbus-specification.html#type-system can be interpreted as an array of structs that contains a string and an int.

Not sure if I hit a new use case or if my interpretation here is incorrect.

@cdoern cdoern changed the title MakVariant cannot properly evaluate a(st) MakeVariant cannot properly evaluate a(st) Jul 5, 2022
@cdoern
Copy link
Author

cdoern commented Jul 5, 2022

@kolyshkin FYI, any help here either with what I am doing wrong or if I can make a PR to fix it would be appreciated.

cdoern added a commit to cdoern/dbus that referenced this issue 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 godbus#328

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@cdoern cdoern linked a pull request Jul 6, 2022 that will close this issue
cdoern added a commit to cdoern/dbus that referenced this issue 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 godbus#328

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@guelfey
Copy link
Member

guelfey commented Jul 6, 2022

Just for my understanding: the issue here is just that the String function returns a bad value, or are you doing something else with it? Note that conceptually, marshalling DBus values as strings and trying to parse them back is not a good idea since that's not standardized, in contrast to the DBus wire format itself. Just the title and the first lines of the issue confuse me: I can see that the formatting is wrong, but MakeVariant and ParseVariant don't have a problem with this type as far as I can tell.

cdoern added a commit to cdoern/dbus that referenced this issue Jul 7, 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 godbus#328

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@cdoern
Copy link
Author

cdoern commented Jul 7, 2022

Just for my understanding: the issue here is just that the String function returns a bad value, or are you doing something else with it? Note that conceptually, marshalling DBus values as strings and trying to parse them back is not a good idea since that's not standardized, in contrast to the DBus wire format itself. Just the title and the first lines of the issue confuse me: I can see that the formatting is wrong, but MakeVariant and ParseVariant don't have a problem with this type as far as I can tell.

I am using MakeVariant to pass form an array of attributes for systemd to place in a unit file. I noticed that my property array contains an attribute that says ["INVALID"]. the format functions seems to be used within the getSignature function so that was the point of improper evaluation. This is my first time looking at the dbus code so I could be wrong but the attached PR tested fixes all of my issues and systemd happily accepts the values

@guelfey
Copy link
Member

guelfey commented Jul 9, 2022

But from looking at containers/common#1082, I can't see why this would cause any issues with what you're trying to achieve. From what I can see, in the end you're just calling StartTransientUnitContext on systemd using the normal D-Bus interface. That should already work regardless of #329, since the communication through D-Bus uses a custom binary format which is completely independent of the string formatting functions that #329 touches.

I'm just trying to figure out where the code lives (if any) that depends on the exact format of the string, because as mentioned, that would still be a Bad Idea (tm).

cdoern added a commit to cdoern/dbus that referenced this issue Aug 11, 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 godbus#328

Signed-off-by: Charlie Doern <cdoern@redhat.com>
guelfey pushed a commit that referenced this issue Sep 21, 2024
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 <cdoern@redhat.com>
guelfey pushed a commit that referenced this issue Sep 21, 2024
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 <cdoern@redhat.com>
guelfey pushed a commit that referenced this issue Nov 9, 2024
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 <cdoern@redhat.com>
guelfey pushed a commit that referenced this issue Nov 9, 2024
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 <cdoern@redhat.com>
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 a pull request may close this issue.

2 participants