Hi Jonas,

On 03/29/2018 05:31 AM, Jonas Bonn wrote:
> On 28/03/18 23:51, Denis Kenzior wrote:
>> Hi Jonas,
>>
>>>
>>> So, that said, this is my attempt at cleaning up service sharing in the
>>> QMI core.  The jist of this is that we switch to a model that looks a
>>> bit like file descriptions/descriptors in Linux.  The QMI services
>>> obtain a service description which manages their info and the underlying
>>> connection (client id) state that the 'shared services' actually share.
>>
>> So can you save me the trouble of looking through 30 patches and 
>> describe what the end resulting API is going to look like?
> 
> Right. So this what I'm aiming for:
> 
> QMI Device -> Service1
>                   -> ServiceHandle1
>                   -> ServiceHandle2
>             -> Service2
>                   -> ServiceHandle1
>                   -> ServiceHandle2
>             -> Service3
>                   -> ServiceHandle1
>                   -> ServiceHandle2
>                   -> ServiceHandle3
>                   -> ServiceHandle4
> 
> ServiceN contains all the shared service data.  This is an underlying 
> structure; it is not made available directly to any user.
> 
> ServiceHandleN is an service instance that requests can be sent to. 
> Notifications and requests are tied to the ServiceHandle they originate 
> on so that they can be properly cancelled when the ServiceHandle i
> 
> This differs from what ofono has today in that today qmi_service_create 
> returns (effecitvely) a reference counted ServiceN instance. qmi_service 
> is a ServiceN object.  In order to not have to change the API, I want to 
> push the shared details of the service down a level and make the user 
> visible qmi_service type be a ServiceHandle.

Okay, so essentially GAtChat (ServiceHandle) and at_chat (Service).

> 
> This is effectively the same as what you proposed with a little twist 
> (improvement :) ):
> i)  you wanted to add a list of groups to qmi_service so that groups of 
> notifations/requests could be cancelled independently of other groups: 
> that what ServiceHandle is above, effectively
Note that MBIM doesn't really have a concept of a service that is 
'opened'.  It is just a UUID, so having everything sit on the device 
object and using groups was more convenient.

> ii)  the improvement is making ServiceHandle the user-visible type 
> instead of ServiceN so that user doesn't need to be aware of this 
> grouping or even the fact that the underlying service is shared... 
> things just work

So same as GAtChat, which is essentially a group id and a pointer to the 
underlying object.

> 
> The reason the patch series becomes long is that reworking the layering 
> in this way is a bit intrusive.  It's worth it though, as the end 
> results is _no API change_.  qmi_service_ref becomes a no-op because the 
> ServiceHandles are not shareable; qmi_service_unref becomes effectively 
> free(ServiceHandle).
> 
> The above is sound.  Now we take things further, just for fun, and put 
> new windows on the house.  Since the above looks like "file 
> descriptions" and "file descriptors" from UNIX, we _could_ use similar 
> terminology.  Just for fun, I proposed a rename:

These are not file descriptors.  You can't cancel an individual send 
request on file descriptors ;)  But anyway, the naming doesn't matter.

> 
> qmi_service_create -> qmi_service_open
> qmi_service_unref -> qmi_service_close
> ...and we drop qmi_service_ref altogether.

We're moving away from reference counting for the most part.  It is a 
glib-ness that we never adopted whole-heartedly and is generally 
pointless in the way oFono is setup.

> 
> Even better would probably be qmi_device_open(device, SERVICE_NAME) 
> because the services (files) live on the device and that's the 'object' 
> we are requesting a service be opend on... but whatever.  The naming 
> isn't important.

I'd just go in the direction of:

uint32_t qmi_device_service_get/open(device, service, callback)
and
qmi_service_free()

Note that the above should be cancellable, otherwise you can crash if 
the atom driver is removed prior to the callback being called.  This is 
a major issue with the existing implementation actually.

> 
> 
>>
>>> On top of this service description we provide service descriptors which
>>> provide private state.  Requests and notifications are registered on the
>>> service descriptor and their lifetimes are tied to it; when the service
>>> descriptor is closed (released), the requests and notifications can be
>>> cleaned up without affecting other users of the shared unlying
>>> description.
>>>
>>
>> This sounds an awful lot like what GAtChat does.  Except less flexible 
>> since you can't unregister or cancel an individual notification/request.
> 
> It doesn't say that anywhere above  Just because I  removed those 
> _unused_ functions in a patch doesn't mean they are impossible to 
> provide.  If there was _one_ user of such a function then having them 
> makes sense, but since they are totally unused they are just untested code.
> 

We should still have it, there are countless examples where this 
capability made things much easier to implement certain behavior.  The 
QMI driver might not use this capability now, but you'll be glad you 
have it when you need it.

Regards,
-Denis