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
352 stars 115 forks source link

1.8: How to use list? #76

Closed whyman closed 4 years ago

whyman commented 6 years ago

This is probably one for @mrjimenez

I've been experimenting with the extra headers (1.8) branch, and am a bit confused to add some items to the list from client code.

There is a method: UpnpFileInfo_add_to_list_ExtraHeadersList(UpnpFileInfo *p, list_head* head);

I was expecting to be able to add a ExtraHeaders* here, how do I make a list_head? Why is it needed?

mrjimenez commented 6 years ago

Hi Ian,

I imagine you already know that, but for completeness, the relevant files are upnp/inc/TemplateSource.h and upnp/inc/TemplateInclude.h, where the methods are generated. And of course, upnp/inc/list.h, where the list implementation resides.

The "list" type used is a frozen version of the linux kernel list. I tried to keep the file as close as possible to the original, so that it would be easy to put patches and keep it up to date.

The clever mechanism in this list is that it can be a list of anything you want, it really does not care about the structure that you want to be in that list. All that matters is that this struct contains a "struct list head something" member anywhere inside it. And it is a pointer to this struct that you get back when you call the get method "const struct list_head CLASS##get##MEMBER(const CLASS p)". All you need to know is that there is a "member function" to retrieve that member object. The object returned is a pointer to a "struct list head". And that list is totally "struct agnostic". You can put anything on that list, as long as it has a field that is a "struct list head" (not a pointer, the struct itself).

To recover your original struct from the list, you must walk that list with the functions and macros of the "list.h" file. For example, two of the most useful macros:

/**

/**

There are several macros there that do the necessary magic. The file list.h is worth a read.

Back when I put this list code in pupnp, the intention was to use something proven and practical and that was written in C, not C++. I think this list code is still used today to implement lots of things inside the linux kernel.

I hope I was clear, but if there are any doubts remaining, please ask.

Best regards, Marcelo.

whyman commented 6 years ago

I am probably doing something wrong but getting:

/usr/include/upnp-1.8/list.h:360:26: note: in definition of macro ‘list_entry’
  container_of(ptr, type, member)
                          ^~~~~~
/usr/include/upnp-1.8/list.h:360:2: error: ‘container_of’ was not declared in this scope
  container_of(ptr, type, member)
  ^
mrjimenez commented 6 years ago

You are doing it right. There is missing code that should be in an include file.

We must add it somewhere, maybe in list.h.

undef offsetof

define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)

/**

mrjimenez commented 6 years ago

I have just committed a new version of list.h, please check.

Also, I did a rebase on the philipe44-master branch, it this is the one you are using, maybe you will want to delete it and fetch it again.

ukleinek commented 6 years ago

My personal opinion is that exposing list_head in the upnp-API is wrong. Something like this is better to be kept internal. The list implementation changed since libupnp 1.6.x, and we have problems like Debian Bug #884243 where programs started to use the upnp list implementation for their own use and so are harder to convert to libupnp 1.8.

whyman commented 6 years ago

@mrjimenez thanks, I will try it later.

@ukleinek I do not see how we can avoid exposing some kind of list here?

ukleinek commented 6 years ago

@v00d00: I'd let the _add routine take either only a single new item or a C array (+ a parameter for the length).

whyman commented 6 years ago

Hmm again probably doing something wrong here, but I cant seem to use this from C++.

I guess this needs an actual C compiler to work.

whyman commented 4 years ago

I am still having troube here with 1.8.5

auto head = const_cast<list_head*>(UpnpFileInfo_get_ExtraHeadersList(fileInfo));
UpnpExtraHeaders* h = UpnpExtraHeaders_new();
UpnpExtraHeaders_strcpy_name(h, "example-header");
UpnpExtraHeaders_strcpy_value(h, "example-value");
UpnpExtraHeaders_set_resp(h, "lolcat");
UpnpExtraHeaders_add_to_list_node(h, head); // ?
// or
UpnpFileInfo_add_to_list_ExtraHeadersList(fileInfo, head); // ?

But nothing seems to be produced

Apolgies in advance...

mrjimenez commented 4 years ago

Hi Ian,

Your code is ok, I don't know why nothing is produced. Both add_to_list's are fine, I just don't have it in my mind right now how the structures are supposed to be used.

In fact, these templates were meant to be used in a C compiler, but there is nothing stopping you from using it from a C++ compiler.

Designing this API for a C++ compiler would in fact simplify things a lot by using real classes.

No need for apologies, if I can help somehow, please tell me.

Regards, Marcelo.

whyman commented 4 years ago

Working for me now... not sure why the above was giving me trouble... PEBKAC almost certainly.

Example of use for adding headers:

auto head = const_cast<list_head*>(UpnpFileInfo_get_ExtraHeadersList(fileInfo));
for (auto header : *headers) {
    UpnpExtraHeaders* h = UpnpExtraHeaders_new();
    UpnpExtraHeaders_set_resp(h, header);
    UpnpExtraHeaders_add_to_list_node(h, head);
}

Do not include a trailing CLRF or you will destroy the HTTP payload :-1:

I wonder if we need to patch the const-ness? Perhaps make CLASS##_add_to_list_##MEMBER take a const value?

whyman commented 4 years ago

And for tidiness consider making ExtraHTTPHeaders() in webserver.c call the UpnpExtraHeaders_add_to_list_node method rather using list_add directly

mrjimenez commented 4 years ago

I think they are both good ideas. If you have a patch I will happily accept it.