godbus / dbus

Native Go bindings for D-Bus
BSD 2-Clause "Simplified" License
976 stars 225 forks source link

`MakeVariant` cannot properly evaluate a(st) #328

Open cdoern opened 2 years ago

cdoern commented 2 years ago

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 commented 2 years ago

@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.

guelfey commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

But from looking at https://github.com/containers/common/pull/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 https://github.com/godbus/dbus/pull/329, since the communication through D-Bus uses a custom binary format which is completely independent of the string formatting functions that https://github.com/godbus/dbus/pull/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).