Hi Denis, On 26 March 2016 at 23:18, Denis Kenzior wrote: > On 03/26/2016 10:50 AM, Andrzej Zaborowski wrote: >> On 26 March 2016 at 04:01, Denis Kenzior wrote: >>> Nah, the signature is fine. I think you need to make the flow a bit >>> simpler. The current logic listens to NameOwnerChanged and also queues >>> up >>> the GetNameOwner call. If the NameOwnerChanged signal wins the race, >>> GetNameOwner call is canceled, connect is called. If GetNameOwner >>> returns >>> first, connect is called and subsequent NameOwnerChanged (if it arrives >>> at >>> all) doesn't do anything. >> >> >> Yep, that's also what I think this patch does, I'm not sure if you're >> saying there's any issue here. We don't cancel the GetnameOnwer >> though. > > > I'm just wondering why we aren't doing the same thing in the new world > order? Canceling GetNameOwner calls would make things much simpler. Let's do it, but I'm not sure about simpler. We're replacing a bool variable with an uint32_t to save the call id, we're issueing an addidional call to l_dbus_cancel and we save a process wakeup, there's little change. > > >> >>> >>> You try to do something similar with pending_get_name_owner flag. So >>> lets >>> use it to our advantage... >> >> >> pending_get_name_owner is necessary to implement the logic above >> because if the watch has just been added (flag is true), we have to >> call connect regardless of whether the service has just popped up or >> has already existed. If the flag is false, we only call connect if >> the service had been disconnected before. >> >>> >>>>> >>>>> >>>>>> + >>>>>> + _dbus_filter_name_owner_notify(dbus->filter, name, new); >>>>>> + >>>>>> + watch = l_hashmap_lookup(dbus->services_watched, name); >>>>>> + >>>>>> + while (watch && !strcmp(watch->bus_name, name)) { >>>>>> + if (watch->removed) >>>>>> + goto next; >>>>>> + >>>>>> + if (watch->pending_get_name_owner) { >>>>>> + watch->pending_get_name_owner = false; >>>>>> + if (new && *new) >>>>>> + watch->connect_func(dbus, >>>>>> watch->user_data); >>> >>> >>> >>> In case new is non-null, reset pending_get_name_owner to false and call >>> connect. >> >> >> We have to additionally check that either the service had been >> disconneted until now (old was empty) or pending_get_name_owner is >> true. >> > > So I think the real issue is that you're trying to take care of two > unrelated code-flows (that perform similar operations) which makes things > pretty hard to understand. I'd be in favor of not hi-jacking the filtering > operations for service watch things. Keep the two path flows separate. Do you mean that _dbus_filter_name_owner_notify is in the same function as the watch handling? I'll move this to a subfunction then. > > And then we should probably add a proper WellKnownName -> UniqueName cache > abstraction, so that we avoid GetNameOwner calls all over the place. > >>> >>>>>> + } else if (connect && watch->connect_func) >>>>>> + watch->connect_func(dbus, watch->user_data); >>>>>> + else if (disconnect && watch->disconnect_func) >>>>>> + watch->disconnect_func(dbus, >>>>>> watch->user_data); >>>>>> + >>>>>> +next: >>>>>> + watch = watch->next; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static void hello_callback(struct l_dbus_message *message, void >>>>>> *user_data) >>>>>> { >>>>>> struct l_dbus *dbus = user_data; >>>>>> @@ -752,7 +797,7 @@ static void get_name_owner_reply_cb(struct >>>>>> l_dbus_message *reply, >>>>>> if (!l_dbus_message_get_arguments(req->message, "s", &name)) >>>>>> return; >>>>>> >>>>>> - _dbus_filter_name_owner_notify(req->dbus->filter, name, >>>>>> owner); >>>>>> + name_owner_notify(req->dbus, name, NULL, owner); >>> >>> >>> >>> Then here, if pending_get_name_owner is false, simply return. >>> get_name_owner_reply lost the race. >> >> >> We don't itereate over all the watches here. We could do that but I >> wanted to extract the common driver-independent logic out to a common >> function, name_owner_notify. > > > Which would iterate anyway, but fair point. > > >> >>> >>> Otherwise, just call with "" instead of NULL and simplify your life in >>> name_owner_notify. >> >> >> But then again there may be watches for the same name that should not >> have their "connect" called, so we still need some way to convey that. >> Besides that "simplification" removes a single NULL check at a cost of >> triplicated code. >> >>> >>>>> >>>>> >>>>> should this use "" instead of NULL? >>>> >>>> >>>> >>>> If we pass "" then name_owner_notify will have to assume this is a new >>>> connection and will issue spurious connect calls. The NULL here says >>>> to not treat this as new connection or disconnection. >>>> >>> >>> And by the way, we might want to handle the case where a service name is >>> changed from one unique name to another. E.g. due to RequestName being >>> queued or DBUS_NAME_FLAG_REPLACE_EXISTING somehow succeeding so that the >>> name is passed from one service to another. >>> >>> For our purposes, calling disconnect and then immediately calling connect >>> would probably be okay for now. >> >> >> Ah again good point, I'll add this. >> >>> >>> >>>>> >>>>>> } >>>>>> >>>>>> static bool classic_get_name_owner(struct l_dbus *bus, const char >>>>>> *name) >>>>>> @@ -800,7 +845,7 @@ static void name_owner_changed_cb(struct >>>>>> l_dbus_message *message, >>>>>> if (!l_dbus_message_get_arguments(message, "sss", &name, >>>>>> &old, >>>>>> &new)) >>>>>> return; >>>>>> >>>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, new); >>>>>> + name_owner_notify(dbus, name, old, new); >>>>>> } >>>>>> >>>>>> static struct l_dbus *setup_dbus1(int fd, const char *guid) >>>>>> @@ -1000,15 +1045,22 @@ static void kdbus_name_owner_change_func(const >>>>>> char *name, uint64_t old_owner, >>>>>> void *user_data) >>>>>> { >>>>>> struct kdbus_message_recv_data *recv_data = user_data; >>>>>> - char owner[32]; >>>>>> + char old[32], new[32]; >>>>>> >>>>>> l_util_debug(recv_data->dbus->debug_handler, >>>>>> recv_data->dbus->debug_data, >>>>>> "Read KDBUS Name Owner Change >>>>>> notification"); >>>>>> >>>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, new_owner); >>>>>> + if (old_owner) >>>>>> + snprintf(old, sizeof(old), ":1.%" PRIu64, new_owner); >>>>> >>>>> >>>>> >>>>> >>>>> old_owner here, not new_owner? >>>> >>>> >>>> >>>> Yes, thanks for catching this. >>>> >>>>> >>>>>> + else >>>>>> + *old = '\0'; >>>>>> + if (new_owner) >>>>>> + snprintf(new, sizeof(new), ":1.%" PRIu64, new_owner); >>>>>> + else >>>>>> + *new = '\0'; >>>>>> >>>>>> - _dbus_filter_name_owner_notify(recv_data->dbus->filter, name, >>>>>> owner); >>>>>> + name_owner_notify(recv_data->dbus, name, old, new); >>>>>> } >>>>>> >>>>>> static struct l_dbus_message *kdbus_recv_message(struct l_dbus >>>>>> *dbus) >>>>>> @@ -1070,15 +1122,13 @@ static bool kdbus_get_name_owner(struct l_dbus >>>>>> *dbus, const char *name) >>>>>> char owner[32]; >>>>>> >>>>>> owner_id = _dbus_kernel_get_name_owner(fd, >>>>>> kdbus->kdbus_pool, >>>>>> name); >>>>>> - if (!owner_id) { >>>>>> - l_util_debug(dbus->debug_handler, >>>>>> - dbus->debug_data, "Error getting name >>>>>> owner"); >>>>>> - return false; >>>>>> - } >>>>>> >>>>>> - snprintf(owner, sizeof(owner), ":1.%" PRIu64, owner_id); >>>>>> + if (owner_id) >>>>>> + snprintf(owner, sizeof(owner), ":1.%" PRIu64, >>>>>> owner_id); >>>>>> + else >>>>>> + *owner = '\0'; >>>>>> >>>>>> - _dbus_filter_name_owner_notify(dbus->filter, name, owner); >>>>>> + name_owner_notify(dbus, name, NULL, owner); >>>>> >>>>> >>>>> >>>>> >>>>> "" instead of NULL? >>>>> >>> >>> With kdbus you don't even have a race, so using "" should be safe, no? >> >> >> Well, name_owner_notify in the current form may call connect for other >> watches for the same service. We'd have to call the connect callback >> right from this function and reset pending_get_name_owner. I >> personally like to keep the whole logic contained in name_owner_notify >> and not have the driver functions do the work, but I could do that. > > > Why not just do the _dbus_kernel_get_name_owner thing right away for kdbus > transports? I think this goes along with what I'm saying above about > keeping the two paths separate. So we're calling dbus->driver->filter_ops.get_name_owner which calls _dbus_kernel_get_name_owner right away, do you mean we should check if dbus->driver == kdbus_ops and then call _dbus_kernel_get_name_owner directly? Or we could add a .get_name_owner method in l_dbus_ops too? Best regards