pupnp / pupnp

libupnp: Build UPnP-compliant control points, devices, and bridges on several operating systems.
https://pupnp.github.io/pupnp
BSD 3-Clause "New" or "Revised" License
353 stars 117 forks source link

UpnpDevice_Handle/UpnpDevice_Handle should be aware of UpnpLib(Handle) #315

Closed whyman closed 2 years ago

whyman commented 3 years ago

On the master branch a bunch of methods now take both a UpnpDevice/Client_Handle and a UpnpLib handle, which is a bit of duplication.

It would be great if the handles returned from UpnpRegisterRootDeviceX contained the parent UpnpLib handle so that clients do not have to track both, and the API could remain unchanged for those methods.

For example: UpnpAcceptSubscriptionExt

mrjimenez commented 3 years ago

Hi Ian,

You have a point. On the other hand, that proposal fact creates a restriction, i.e., if one application wants to create multiple devices and/or multiple clients, which was a "feature" of the old library we currently support, it will have to do so by creating multiple library handles.

There is currently a struct Handle_Info and a handle_table_t that were meant to support that. Which brings to my mind a strange artifact that test_init.c shows, that is a dump of the handle table every time that a message is received that we probably should check. But back to the topic:

typedef enum
{
        HND_INVALID = -1,
        HND_CLIENT,
        HND_DEVICE
} Upnp_Handle_Type;

Maybe we could remove this multi-{client/device} support and keep one single handle, be it a device or a client handle? I really don't know, I have not figured out all the consequences, what do you think?

whyman commented 3 years ago

I think multi-device support must remain.

My idea was more along the line of converting the UpnpDevice/Client_Handles to a struct or something that can encapsulate the library handle, so the library handle could still be used in the implementation

mrjimenez commented 3 years ago

Sounds ok to me. Some interfaces would have a library handle, other the client/device handle. I can live with that, it is not that ugly.

The solution you propose is actually easy to implement. Device and Client handles are currently an index into the handle table. That table is an array of pointers to structs, so it would only be a matter of including the library handle into the struct.

I would go just a little bit further and eliminate the difference between the client and the device structs. Even though they are different, they are kept at the same array and it requires a series of ifdefs spread along the code. Instead of the ifdefs in the struct, I would only leave it commented what is client and what is device. struct Handle_Info already has a member Upnp_Handle_Type that differentiates them. A (near?) future object oriented implementation could use a virtual class if there is any advantage in the code.

So, in order not to run over you, this time I will ask first: will you do it? :laughing:

Regards, Marcelo.

whyman commented 3 years ago

I am pretty time limited at the moment, so feel free to go ahead. If it is not completed by the time I get some more time then I will a have look.