Bug 106712 - Containers (#100344): consider changing instance ID from 'o' to 'x'
Summary: Containers (#100344): consider changing instance ID from 'o' to 'x'
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: All All
: medium enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 100344
  Show dependency treegraph
 
Reported: 2018-05-29 15:03 UTC by Simon McVittie
Modified: 2018-10-12 21:34 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2018-05-29 15:03:38 UTC
+++ This bug was initially created as a clone of Bug #100344 +++

Allison would prefer for container instance IDs to be integer handles, as they were in the initial designs at a hackfest. They are currently object paths (but as an implementation detail, the varying part of the object path is in fact a dbus_uint64_t).

Justifications for preferring object paths:

* arg0 matching currently only works on stringy types
  - In particular, services that monitor the lifetimes of container
    instances might well want to match ContainerRemoved signals whose
    arg0 is a container of interest
* GDBus' API for arg0 matching takes a const gchar *, so matching
  integers would require new C API (or the caller providing a
  canonical stringification, but that's an API wart)
* We tell D-Bus API designers to use object paths for their opaque
  handles (like MPRIS2 tracks, but unlike Telepathy's 32-bit unsigned
  integer handles, which with hindsight are considered to have been
  an anti-pattern) and it's a bit hypocritical not to do the same
  thing ourselves
* (more?)

Justifications for preferring integers:

* They're smaller/more efficient
* We can format them into directory hierarchies, like
  $XDG_RUNTIME_DIR/containers/%lld/dconf-db, without it resulting in
  monstrously long filenames
* (more?)

Discuss.

I am not going to change the implementation unless consensus is reached, but I am also intending to leave the implementation counter-based so that switching to dbus_uint64_t would be straightforward.
Comment 1 Simon McVittie 2018-05-29 15:10:24 UTC
(In reply to Simon McVittie from comment #0)
> * We can format them into directory hierarchies, like
>   $XDG_RUNTIME_DIR/containers/%lld/dconf-db, without it resulting in
>   monstrously long filenames

Possible rebuttal for this:

Do we expect that portal-like services like dconf will be able to support various container managers (Flatpak, Snap, etc.) generically, or do we expect them to need container-manager-specific code?

If they need container-manager-specific code *anyway*, then perhaps they can (and maybe should?) use the container name (e.g. the app ID like com.example.App when using Flatpak) in filenames like this, rather than the instance ID?
Comment 2 David Herrmann 2018-06-29 10:15:25 UTC
(In reply to Simon McVittie from comment #0)
> Justifications for preferring object paths:
> 
> * arg0 matching currently only works on stringy types
>   - In particular, services that monitor the lifetimes of container
>     instances might well want to match ContainerRemoved signals whose
>     arg0 is a container of interest

Meh. There is no reason why we only support string-matching, other than lack of time and use-cases to implement others, right?

I agree, as long as no-one steps up to implement it, this is a valid argument.

> * GDBus' API for arg0 matching takes a const gchar *, so matching
>   integers would require new C API (or the caller providing a
>   canonical stringification, but that's an API wart)

New code comes with new symbols. I am fine with that.

> * We tell D-Bus API designers to use object paths for their opaque
>   handles (like MPRIS2 tracks, but unlike Telepathy's 32-bit unsigned
>   integer handles, which with hindsight are considered to have been
>   an anti-pattern) and it's a bit hypocritical not to do the same
>   thing ourselves

We can always reconsider :)

> Justifications for preferring integers:
> 
> * They're smaller/more efficient

I doubt that they're more efficient. CPUs can deal with strings just fine. And 64bit integers are still 7-character C-strings, so lots of space in there.

> * We can format them into directory hierarchies, like
>   $XDG_RUNTIME_DIR/containers/%lld/dconf-db, without it resulting in
>   monstrously long filenames

Yes.

> * (more?)

I like integers because their behavior is predictable. This is the biggest argument I have in favor of them. You know their size, you know their scope, you can enumerate them, you can deal with them with simple, fast code. No string-parsing required, so string-verification, no UTF-8 validation, no escaping/unescaping, *NO* invalid possibilities (all possible values are valid values).

Clients cannot spam you with ridiculously large strings (ok, in case of object_path we have the 255 limit, so that's something), or trigger concatenation errors (imagine you embed the object-path in another object-path, or string, or URL, etc).

Lastly, I don't want to invent stringified names for things. I don't name my pointers in a C program, so I don't want to name my D-Bus objects. I can see that there *are* situations where you have singletons or root objects that might benefit of having names for debuggability/discoverability. However, for all transient objects, I would prefer not being required to name them and then suffix them with some random-integer that I count up for each object created.

I don't mind if we continue using object-paths. This is D-Bus legacy and it works, we learned how to deal with it. But if I were to choose, I would always prefer the integer.

Cheers
David
Comment 3 Simon McVittie 2018-08-30 19:47:41 UTC
(In reply to David Herrmann from comment #2)
> Meh. There is no reason why we only support string-matching, other than lack
> of time and use-cases to implement others, right?

With string matching, it's pretty obvious what does and doesn't match.

Integer matching between a pattern (from e.g. arg0int='PATTERN' in a match rule) and a candidate value (in a message) in the presence of differently-typed integers is a can of worms. I see these options:

1) Canonical stringification: store the pattern as a canonical
   string (e.g. base 10, no leading 0 unless zero, no -0), convert the
   candidate value to the same canonical string, and compare them with
   strcmp()

2) Arbitrarily say we will only match integers of a specific type, and
   not the other types; if the candidate value is a uint32 (as in
   Telepathy) but we only support matching uint64s, then you lose

3) Arbitrarily say we will match integers of any type, but only if the
   pattern fits in a specific type (dbus_int64_t or dbus_uint64_t);
   if you want your pattern to be very large or negative (whichever one
   we don't choose) because it has some sort of internal structure
   (e.g. you got it from the kernel or another process), then you lose

4) Store the pattern as some sort of tagged union,
   struct { bool negative;
   union { largest_int if_negative; largest_uint if_not_negative; } value; },
   and do a relatively elaborate comparison between the candidate value
   and the pattern

(In current D-Bus, largest_int is dbus_int64_t and largest_uint is dbus_uint64_t, although we might want 128-bit integers one day.)

> > * GDBus' API for arg0 matching takes a const gchar *, so matching
> >   integers would require new C API (or the caller providing a
> >   canonical stringification, but that's an API wart)
> 
> New code comes with new symbols. I am fine with that.

Match rules are also string-based (AddMatch() takes a string), so we end up with a stringification at some point either way. Unless we add an AddMatch2(a{sv}), but I don't want to make that a prerequisite for Containers1, because if I start adding more prerequisites I'll never finish it.
Comment 4 GitLab Migration User 2018-10-12 21:34:52 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/210.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.