There have been requests and suggestions for a "maybe" or nullable type in the past, and so I propose the following: "maybe", literal "m", is a container type much like an array, but it can only hold either 0 or 1 element. So "ms" could be either "spam", "" or null. Think of 'string? eggs' in Vala or C#, where the question mark indicates that the string can be null.
Created attachment 35306 [details] [review] Implement "maybe", "m", DBUS_TYPE_MAYBE This patch implements DBUS_TYPE_MAYBE. It's a container. It behaves like an array. The macro DBUS_TYPE_IS_ARRAY tests whether a type literal is array-like, which is either 'a' or 'm'. In a lot of places array was hard-coded, and the macro avoids cluttering the code with repeated tests for the new type. The macro is in dbus-protocol.h right now. I'm not sure if it should actually be public, it might be useful. Mainly this was the most convenient place to add for the moment. I changed dbus_message_append_args_valist to "inherit" the array or maybe type it was given instead of assuming it's an array. dbus_message_iter_append_fixed_array checks that a maybe has 0 or 1 element. Unfortunately, there is one big flaw: all maybe types end up as arrays after being processed. I haven't yet found out how to solve that. writer_recurse_array, _dbus_type_writer_append_array and _dbus_header_create still hardcode DBUS_TYPE_ARRAY. I didn't see a way to change that as I didn't see how to find out the right type, I'm guessing that could relate to the unwanted convertion.
By the way, for comparaison, GVariant already supports a maybe type http://library.gnome.org/devel/glib/stable/glib-GVariantType.html#G-VARIANT-TYPE-MAYBE:CAPS .
Created attachment 35341 [details] [review] Implement "maybe", "m", DBUS_TYPE_MAYBE So I solved the mentioned problem and I can actually send and receive maybes. And it also enforces that you put only one element inside the maybe and fails otherwise.
Review of attachment 35341 [details] [review]: Your patch is one long diff. Please create Git commits and attach the output of git format-patch instead. In doing that, please split the patch into multiple, logical commits, like the one for changing the array checks into DBUS_TYPE_IS_ARRAY. Also, please include the maybe types in the feature negotiation system in the handshake process as well as a flag to be enabled on the DBusConnection prior to connecting. "Maybe" types should be supported if and only if: - the binding declared it supports "maybe" types prior to connecting - both ends of the connection declared they support "maybe" types in the handshake phase. Please ensure that trying to send a message containing a "maybe" type over a connection where they aren't allowed results in an error generated. This also means the bus must ensure this happens when delivering a message from one client to the next: the message can only be delivered if both connections had "maybe" types enabled and so did the bus. This functionality is already present in D-Bus code for the file-descriptor-passing functionality.
I haven't looked at the patch. My huge concern with this is that all libdbus-consuming bindings will need to be updated. It doesn't help if libdbus says when negotiating a connection "I support maybe types" but the high level binding just aborts (as e.g. dbus-glib will) when wrapped on top.
Which is why I said the binding must enable the feature in the connection before the negotiation happens.
Created attachment 35454 [details] [review] Use generic DBUS_TYPE_IS_ARRAY for array checks
Created attachment 35455 [details] [review] Implement DBUS_TYPE_MAYBE, 'm', "m"
Created attachment 35456 [details] [review] Check that maybe has none or one element
Created attachment 35457 [details] [review] Include DBUS_TYPE_IS_ARRAY in documentation
Created attachment 35458 [details] [review] Implement negotiation for maybe passing
(In reply to comment #11) > Created an attachment (id=35458) [details] > Implement negotiation for maybe passing As proposed on IRC, it's not really cool to have this extra ping-pong step.. and when we add support for 'f' (float) it's yet another one... instead, let's scrap the existing NEGOTIATE_UNIX_FD/AGREE_UNIX_FD and just introduce C->S: NEGOTIATE_CAPABILITIES cap1,cap2,cap3,... S->C: AGREE_CAPABILITIES acap1,acap2,acap3,... where the returned caps are a subset of the passed caps. I suggest using capability names unix-fd and maybe for now. I'm not married to the term 'capability' btw, feel free to use 'feature' or something else. Thoughts?
(In reply to comment #5) > I haven't looked at the patch. My huge concern with this is that all > libdbus-consuming bindings will need to be updated. It doesn't help if libdbus > says when negotiating a connection "I support maybe types" but the high level > binding just aborts (as e.g. dbus-glib will) when wrapped on top. (Some rambling ahead, sorry, didn't have time to write a shorter more concise message.) Well, first of all, this can only happen when handling variant values (ecause non-variant values in pre-type-m bindings (or applications) will never support type 'm' anyway. So a simple signature check will save the app.) Anyway, if the binding (or application) is written with two important facts in mind (1. that D-Bus is an extensible protocol; 2. defensive programming), it should always check the actual type of a variant value before doing anything. For example, typical implementations of org.freedesktop.DBus.Properties.GetProperty(String name, Variant v) checks that the actual type of @v matches that of the property with the name @name. So if someone sends a value of type 'v' and it happens to contain a maybe type then the binding (or application) should just ignore it as the type 'm' is not known to the binding (or application) at the time it was written. So if any binding or application breaks because of this it was probably broken already, no? Anyway, trying to conclude here, sure, maybe a couple of bindings (or applications) will need to be updated to be more careful when handling variants (which I think belongs to the realm of bug-fixing). In reality I don't think (non-hostile) apps are going to be sending maybe types as variants to services that don't expect them anyway. We need to be careful though... in the system bus case, being able to crash a privileged service is usually a security issue.
Bindings may try to demarshall the entire message, including variants, to their native types. When encountering an unknown type, bad things could happen. In addition to that, there are many applications using libdbus-1, especially those on the system bus. So I stand by what I suggested before: add a "flags" argument to DBusConnection that enables extra features. Something like: - dbus_get_with_flags - dbus_get_private_with_flags - dbus_connection_open_with_flags - dbus_connection_open_private_with_flags
Created attachment 35597 [details] [review] Implement optional DBUS_CAPABILITY_FLAGS This patch implements DBusCapabilityFlags with values for Unix FD and Maybe, _with_flags functions as suggested above as well as dbus_connection_get_flags to find out which capabilities were succesfully supported. The way it works is that the new types are disabled by default, and clients have to enable them, otherwise they will be treated as invalid when sending. The types are enabled on the server side. I changed the interal connection functions to take a flags argument and use _dbus_transport_set_capabilities internally to add the flags.
Comment on attachment 35597 [details] [review] Implement optional DBUS_CAPABILITY_FLAGS >diff --git a/dbus/dbus-auth.c b/dbus/dbus-auth.c >index 5648d60..0f690f7 100644 >--- a/dbus/dbus-auth.c >+++ b/dbus/dbus-auth.c >@@ -2141,13 +2141,15 @@ handle_client_state_waiting_for_agree_unix_fd(DBusAuth *auth, > _dbus_assert(auth->unix_fd_possible); > auth->unix_fd_negotiated = TRUE; > _dbus_verbose("Sucessfully negotiated UNIX FD passing\n"); >- return send_negotiate_maybe (auth); >+ return auth->maybe_possible >+ ? send_negotiate_maybe (auth) : send_begin (auth); Can you redo this patch as a preliminary step not dependent on your maybe types changes? In other words, the maybe types patch should introduce a new capability on top of this one. I agree with davidz about having the negotiation happen in one pass.
Created attachment 36154 [details] [review] Rework NEGOTIATE_UNIX_FD as NEGOTIATE_CAPABILITIES These are the protocol changes without Maybe.
Created attachment 36155 [details] [review] Implement optional DBUS_CAPABILITY_FLAGS This is DBusCapabilityFlags and new connection/ bus _with_flags functions without Maybe.
Just to confirm, the protocol encoding of maybe in this patch is the same as for GVariant, right? (or is GVariant so far out of sync with dbus that it doesn't matter?)
(In reply to comment #19) > Just to confirm, the protocol encoding of maybe in this patch is the same as > for GVariant, right? (or is GVariant so far out of sync with dbus that it > doesn't matter?) It is 'm' just like in GVariant and has the same semantics.
(In reply to comment #19) > Just to confirm, the protocol encoding of maybe in this patch is the same as > for GVariant, right? (or is GVariant so far out of sync with dbus that it > doesn't matter?) GVariant and D-Bus uses different binary encodings so it doesn't matter (see gio/gdbusmessage.c for the encoders/decoders - yes, very unoptimized at the moment).
Review of attachment 35454 [details] [review]: ::: dbus/dbus-protocol.h @@ +129,3 @@ + (typeliteral == DBUS_TYPE_ARRAY \ + || typeliteral == DBUS_TYPE_MAYBE) + I can see why it's attractive to implement Maybe like this, but I think this macro is misleadingly named. I think it should be _DBUS_TYPE_IS_ARRAYLIKE or similar, and should not be public. It's really an implementation detail of the marshaller and demarshaller.
Just to reiterate comment #5, I'd feel a lot more comfortable with this patch if some basic analysis of the impact on both dbus-glib and dbus-python had been done. Do they need to be updated?
Review of attachment 35455 [details] [review]: There's a typo in the name of this patch. :) ::: dbus/dbus-message.c @@ +2622,3 @@ _dbus_return_val_if_fail (dbus_type_is_fixed (element_type) && element_type != DBUS_TYPE_UNIX_FD, FALSE); + _dbus_return_val_if_fail (real->u.writer.container_type == DBUS_TYPE_ARRAY + || (real->u.writer.container_type == DBUS_TYPE_MAYBE && n_elements < 2), FALSE); I'd be happier if this check was two separate checks: one using _DBUS_TYPE_IS_ARRAYISH; and a second: if (real->u.writer.container_type == DBUS_TYPE_MAYBE) _dbus_return_val_if_fail (n_elements < 2), FALSE); This would make it easier to tell what's happened when debugging assertion failures.
(In reply to comment #23) > Just to reiterate comment #5, I'd feel a lot more comfortable with this patch > if some basic analysis of the impact on both dbus-glib and dbus-python had been > done. Do they need to be updated? Bindings need to be updated if they support Maybe types. Which makes sense, because I see no way to pass something the binding doesn't bind.
Review of attachment 35456 [details] [review]: ::: dbus/dbus-message.c @@ +890,3 @@ + if (spec_type == DBUS_TYPE_MAYBE && _DBUS_UNLIKELY (n_elements > 1)) + { + dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, I think this error should be DBUS_ERROR_INCONSISTENT_MESSAGE, like the not-enough-FDs error above. @@ +2694,3 @@ _dbus_check_is_valid_signature (contained_signature)), FALSE); + /* the containted signature of a maybe must contain exactly one element */ "containted" — typo for "contained". But correct me if I'm wrong (I find this function very hard to follow) but it seems like you're requiring the contents of a Maybe to be only a basic type. This seems like an unnecessary restriction. @@ +2698,3 @@ + (contained_signature[0] != '\0' && + contained_signature[1] == '\0')) || + type != DBUS_TYPE_MAYBE), FALSE); As a stye point, this is again not a particularly good way to phrase this assertion. You should say if (type == DBUS_TYPE_MAYBE) _dbus_return_val_if_fail(...)
Review of attachment 35458 [details] [review]: ::: dbus/dbus-auth.c @@ +195,3 @@ unsigned int unix_fd_negotiated : 1; /**< Unix fd was successfully negotiated */ + + unsigned int maybe_possible : 1; /**< This side could do maybe passing */ "maybe passing" feels like it's been cargo-culted from the unix fd stuff above. "This side supports the Maybe type" would be better. @@ +2218,3 @@ + { "AGREE_UNIX_FD", DBUS_AUTH_COMMAND_AGREE_UNIX_FD }, + { "NEGOTIATE_MAYBE", DBUS_AUTH_COMMAND_NEGOTIATE_MAYBE }, + { "AGREE_MAYBE", DBUS_AUTH_COMMAND_AGREE_MAYBE } The alignment of the second struct members here is inconsistent with the stuff above. ::: dbus/dbus-transport.c @@ +834,3 @@ + */ +dbus_bool_t +_dbus_transport_can_pass_maybe (DBusTransport *transport) Why is this function necessary? It's needed for passing FDs because you can't pass them over TCP sockets. But you can certainly pass values of type 'Maybe a' over anything. So every transport can relay them.
Review of attachment 35597 [details] [review]: Well, 1.4.0 always enables FD passing, so this patch will need reworking a bit. In any case... ::: dbus/dbus-bus.c @@ +596,3 @@ + * flags are only meaningful if the connection is newly-created, + * otherwise they have no effect. You have to check the supported + * capabilities afterwards with dbus_connection_get_capabilities(). Okay, so there's a problem here if two libraries share the same D-Bus connection, one which understands Maybe and one which does not. If the first one turns on Maybe, the second one doesn't have any choice in the matter. I think in practice this is only a problem for dbus-glib. No other binding based on libdbus (that I know of) uses the shared connection. You could imagine a situation where an application uses dbus-glib, and also uses a library that uses libdbus directly... I guess this is reasonably theoretical.
There should also be a spec patch that documents this Maybe type. Have you checked how GVariant represents Maybe in messages? Its serialization format is not exactly the same as DBus's IIRC but it's probably worth making this the same if it isn't already.
Created attachment 41288 [details] [review] Internal _DBUS_TYPE_IS_ARRAYLIKE for array checks (In reply to comment #22) > Review of attachment 35454 [details] [review]: > macro is misleadingly named. I think it should be _DBUS_TYPE_IS_ARRAYLIKE or > similar, and should not be public. It's really an implementation detail of the > marshaller and demarshaller. Changed to _DBUS_TYPE_IS_ARRAYLIKE and moved to dbus-internal.h.
Created attachment 41289 [details] [review] Implement DBUS_TYPE_MAYBE, 'm', "m" (In reply to comment #24) > Review of attachment 35455 [details] [review]: Changed assertion for clarity and fixed typo.
Created attachment 41290 [details] [review] Implement negotiation of Maybe type support > +_dbus_transport_can_pass_maybe (DBusTransport *transport) > > Why is this function necessary? It's needed for passing FDs because you can't > pass them over TCP sockets. But you can certainly pass values of type 'Maybe a' > over anything. So every transport can relay them. To avoid breaching the transport API, since that's what keeps the value.
Created attachment 41292 [details] [review] Implement optional DBUS_CAPABILITY_FLAGS (In reply to comment #28) > Review of attachment 35597 [details] [review]: > > Well, 1.4.0 always enables FD passing, so this patch will need reworking a bit. In fact not, this was last updated when FD's existed in their current form. > + * flags are only meaningful if the connection is newly-created, > + * otherwise they have no effect. You have to check the supported > + * capabilities afterwards with dbus_connection_get_capabilities(). I added to the documentation that unix FDs are by default enabled. > Okay, so there's a problem here if two libraries share the same D-Bus > connection, one which understands Maybe and one which does not. If the first > one turns on Maybe, the second one doesn't have any choice in the matter. > > I think in practice this is only a problem for dbus-glib. No other binding > based on libdbus (that I know of) uses the shared connection. You could imagine > a situation where an application uses dbus-glib, and also uses a library that > uses libdbus directly... I guess this is reasonably theoretical. I don't think there's much we can do at all. Using a shared connection with different bindings has always been tricky, and who does this has to deal with it.
Created attachment 41294 [details] [review] Check that a maybe has none or one element (In reply to comment #26) > Review of attachment 35456 [details] [review]: Changed to DBUS_ERROR_INCONSISTENT_MESSAGE Typo fixed. > As a stye point, this is again not a particularly good way to phrase this > assertion. You should say if (type == DBUS_TYPE_MAYBE) > _dbus_return_val_if_fail(...) This so the check disappears if checks are disabled, like everywhere else. I tend to think this should be re-considered everyhwere if it is desirable. > But correct me if I'm wrong (I find this function very hard to follow) but it > seems like you're requiring the contents of a Maybe to be only a basic type. > This seems like an unnecessary restriction. Indeed, I was unsure at the time how to do this, somehow I had been oblivious to dbus_signature_validate_single... Fixed now.
Created attachment 41296 [details] [review] Implement negotiation of Maybe type support Fixed patch, the previous one was incorrect.
-- 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/25.
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.