Extra Headers

[philippe44]

Tring to add the modifications I've made in 1.6 to 1.8, but the way things are defined has changed. I just had a look and it seems that now what is defined in header files is build automatically, but I don't know how. I'm missing the structure: struct Extra_Headers { /** The length of the file. A length less than 0 indicates the size

• is unknown, and data will be sent until 0 bytes are returned from
• a read call. / char name; char *value; DOMString resp; };

That used to be dfined in upnp.h, now it should be added to FileInfo.h, but I'm not sure.

[whyman]

@mrjimenez If I recall you added these structures, do you care to comment on this patch?

[ukleinek]

Disclaimer: I don't understand what you want to achieve, but I wonder if this should better go to the 1.8 branch instead?!

Edit: this is for the 1.8 branch, sorry for the noise

[whyman]

@ukleinek also FYI equivalent of this patch is already merged to 1.6

[ukleinek]

Yeah, I saw that, and that's what I intended to object against. AFAIK 1.6 is maintenance only, and so I'd not add new features to it but better make applications convert to 1.8 to get new features.

[whyman]

@ukleinek I agree entirely - personally I would not have merged it into 1.6, but 1.8.

[mrjimenez]

Hi Ian, hi Uwe,

The patch is work in progress. Extra headers have been added to 1.6 as a result of Philippe's work, even though 1.6 is maintenance only. There were no objections when he did that, quite the opposite, so I have accepted his patch at the time. But of couse, added the functionality should go into 1.8 some day.

That new patch is an attempt to give Philippe's a starter point to port his changes to 1.8. The 1.8 template way might be a little confusing, it is almost a dialect of C++ to define opaque objects. 1.6 had lots of repeated code and exposed internal structures and that made developers life miserable, new libtool release numbers for incompatible libs had to be made constantly, and these are a small nightmare to keep consistent and without errors.

External headers added an extra struct, which is acceptable in the 1.6 context, but is not acceptable in 1.8 context, since it exposes internal library data. By the way, just a side note, the new 1.6 libtool numbers must reflect that change (added interface).

But I might be missing the point, do you think I did something wrong? The patch I did is supposed to go on a separate branch to be used by Philippe and should not yet be merged into 1.8 until he finishes.

Regards, Marcelo.

[whyman]

Marcelo,

I dont think you did anything wrong at all, I think both @ukleinek and myself were just expecting "new stuff" like this patch to be merged into 1.8 in preference of 1.6.

I simply am keen to see this in 1.8.x - where as right now, 1.6 has more features than 1.8!

Thanks for your work on pupnp, its appreciated!

[whyman]

Is there anything outstanding on this still? Or is it good to merge?

[philippe44]

I've not been able to work on this yet - I'm super busy right now (releasing two other apps including one using libupnp) and as said by Marcelo, I need a different approach in 1.8 vs 1.6 as the extra headers must be a linked list, which also means a different interface. I also need to build test and so on. All this during weekends and evenings :-(

[whyman]

@philippe44 I get it absolutely, I am in the same boat. Thanks for your efforts! I'm just hoping to use this functionality is all :+1:

[whyman]

What work is outstanding here? I am happy to do the work if someone tells me what needs doing?

[mrjimenez]

Hi Ian,

I suppose that the last patch I did should be the missing part, it added the possibility of using a list of extra headers instead of a single extra header using the template "magic". Of course, it must be tested, and that is something I did not do.

I am not aware of any other issues, but it would certainly be advisable to check the changes made in 1.6.x to see if all the functionality has been added.

If I recall correctly, Philippe has already pulled my patch into his tree, so it should not be difficult to test his tree if it is compiling. The greatest change relative to 1.6.x code is probably that you do not have an array, you have a list of Extra Headers, so the way to iterate is different, but the code that does the processing should be the same.

Easy said, but I did not try that myself. Your offer is more than welcome!

Best regards, Marcelo.

[whyman]

@mrjimenez I notice that the new ExtraHTTPHeaders() method doesn't seem to get called?

[mrjimenez]

More on the extra-headers front:

I have just created an "extra-headers" branch that is the work of @philippe44 rebased on top of master a.k.a. 1.8.x.

Since it was very well pointed by @medoc92 that extra-headers is a 1.6 feature that is not present in 1.8, I would very much like to integrate it, since it might be one reason to hold a program to go from 1.6 to 1.8. As far as I can tell, it should be ok, but unfortunately I have no reports of people testing it, and I myself currently have no resorts to test it.

I certainly would prefer to have the code tested before incorporated, but since it is new stuff that isn't likely to break old stuff, maybe we could incorporate it and wait for the bug reports from the first people to use it. That problem has been also discussed in #93, but I believe this is the proper thread to discuss it.

Regards, Marcelo.

[ghost]

I tried the new branch and it works fine for me with one minuscule issue. As the branch is separate from the pull request, I'm not sure how to comment on it, so here:

in TemplateInclude.h in "replace_in_list()" (line 76). A parameter named 'new' is used. 'new' is a reserved keyword in c++ so this needs to be replaced with 'newent' or whatever.

In httpreadwrite.c http_MakeMessage(), 2 cosmetic remarks: the 'E' case is not part of the big ifelse. In practise this does not change anything, but the reader loses time wondering why this is the case. Just handle it like the other letters I think ? Also this saves a test (maybe?) :=)

Also, it seems to me that there is no real warranty that the 'head' value will always be not null (with possible future callers), so I'd guard against it, like in 1.6 (I did read the code in the current caller and it's ok, but a bit of defensive programming is often a good thing).

[mrjimenez]

I have just committed your suggestions. May I put the extra-headers code in master?

I have one more issue, but that can be resolved later:

/home/user/pupnp/github/upnp/src/genlib/net/http/webserver.c:1042: warning: ‘ExtraHTTPHeaders’ defined but not used [-Wunused-function] static int ExtraHTTPHeaders( ^~~~

That function is currently never called.

Regards, Marcelo.

[whyman]

If you look at the 1.6 version, it looks like we need a version of:

https://github.com/mrjimenez/pupnp/blob/2445f1bb2e2690aca34accc6a015310f01d36b1f/upnp/src/genlib/net/http/webserver.c#L1188-L1194

[mrjimenez]

Ian,

Ok, done! Anything else? Anyone?

The branch extra-headers has been updated. I am really tempted to merge it to 1.8. How about doing it tomorrow?

Regards, Marcelo.

[philippe44]

Ultimately, is anybody using ExtraHeader? I'm even surprised that upmpdcli crashed because of it, does it use the "E" parameter but sends no argument?

Anyway, I did that, with a lot of mistakes, when I was restarting coding after an extremely long (...) pause, so as demonstrated with the various issues, it was (is) bad quality. Maybe just removing it from any version is the proper decision

[ghost]

Thanks ! And I can see no problem with getting this into master.

OOpsee.. this was last night, I now have a crash in the new webserver.c code. `

0 list_add (head=0x0, newent=) at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/inc/list.h:130

1 ExtraHTTPHeaders (extraHeadersList=, Req=) at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/src/genlib/net/http/webserver.c:1069

2 process_request (RespInstr=0x7f11c0852978, alias=, filename=0x7f11c0852958, headers=0x7f11c0852938, rtype=, req=0x7f11c0852c18)

at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/src/genlib/net/http/webserver.c:1182

3 web_server_callback (parser=, req=0x7f11c0852c18, info=) at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/src/genlib/net/http/webserver.c:1560

4 0x00007f11c8c0e4cd in dispatch_request (hparser=0x7f11c0852c18, info=0x7f11c0852b90) at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/src/genlib/miniserver/miniserver.c:157

5 handle_request (args=0x7f11b0000b20) at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/src/genlib/miniserver/miniserver.c:230

6 0x00007f11c8c07975 in WorkerThread (arg=0x7f11c8e2e1e0 ) at /y/home/dockes/projets/mpdupnp/pupnp/pupnp/upnp/src/threadutil/ThreadPool.c:576

`

As far as I can see, ExtraHTTPHeaders() is called with a null extraHeadersList parameter and list_add does not expect a null head. The parameter comes from extra_headers in process_request() which indeed appears to be initialized to null and never allocated.

[ghost]

Ultimately, is anybody using ExtraHeader? I'm even surprised that upmpdcli crashed because of it, does it use the "E" parameter but sends no argument?

If I remember well, the problem was precisely that it would crash with any app which did not use the feature :)

Anyway, I did that, with a lot of mistakes, when I was restarting coding after an extremely long (...) pause, so as demonstrated with the various issues, it was (is) bad quality. Maybe just removing it from any version is the proper decision

I think that the feature is already in use actually, and it's a nice one.

In any case, we all make mistakes while programming !

The problem I tried to highlight is one of process. Nobody's fault, but, as a group of programmers, it is my opinion that we need to agree on a slightly more structured and prudent pipeline for changes to ultimately get their way into packages for Debian stable or Fedora.

[ghost]

As far as I can see, the following patch clears the latest crash. I see no reason for the list_head to be dynamically allocated, but maybe I'm wrong, maybe check:

diff --git a/upnp/src/genlib/net/http/webserver.c b/upnp/src/genlib/net/http/webserver.c
index d59e402..f0f3f5b 100644
--- a/upnp/src/genlib/net/http/webserver.c
+++ b/upnp/src/genlib/net/http/webserver.c
@@ -1116,8 +1116,9 @@ static int process_request(
        int resp_minor;
        int alias_grabbed;
        size_t dummy;
-       struct list_head *extra_headers = NULL;
-
+       LIST_HEAD(extra_headers_store);
+       struct list_head *extra_headers = &extra_headers_store;
+       
        print_http_headers(req);
        url = &req->uri;
        assert(req->method == HTTPMETHOD_GET ||

[mrjimenez]

I will close this PR, since it is already merged and will not happen via github. But please, feel free to open another extra-headers issue, the code is pretty fresh.