Re: [RFC PATCH 00/30] QMI rework

[Jonas Bonn <jonas@southpole.se>]

[-- Attachment #1: Type: text/plain, Size: 4157 bytes --]

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.

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

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:

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

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.


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

/Jonas

>
> Regards,
> -Denis