New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra Headers #49

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@philippe44

philippe44 commented Jul 27, 2017

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.

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Aug 4, 2017

Contributor

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

Contributor

v00d00 commented Aug 4, 2017

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

@ukleinek

This comment has been minimized.

Show comment
Hide comment
@ukleinek

ukleinek Aug 4, 2017

Contributor

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

Contributor

ukleinek commented Aug 4, 2017

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

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Aug 4, 2017

Contributor

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

Contributor

v00d00 commented Aug 4, 2017

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

@ukleinek

This comment has been minimized.

Show comment
Hide comment
@ukleinek

ukleinek Aug 4, 2017

Contributor

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.

Contributor

ukleinek commented Aug 4, 2017

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.

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Aug 4, 2017

Contributor

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

Contributor

v00d00 commented Aug 4, 2017

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

@mrjimenez

This comment has been minimized.

Show comment
Hide comment
@mrjimenez

mrjimenez Aug 4, 2017

Owner

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.

Owner

mrjimenez commented Aug 4, 2017

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.

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Aug 4, 2017

Contributor

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!

Contributor

v00d00 commented Aug 4, 2017

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!

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Aug 20, 2017

Contributor

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

Contributor

v00d00 commented Aug 20, 2017

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

@philippe44

This comment has been minimized.

Show comment
Hide comment
@philippe44

philippe44 Aug 21, 2017

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

philippe44 commented Aug 21, 2017

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

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Aug 21, 2017

Contributor

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

Contributor

v00d00 commented Aug 21, 2017

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

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Jan 14, 2018

Contributor

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

Contributor

v00d00 commented Jan 14, 2018

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

@mrjimenez

This comment has been minimized.

Show comment
Hide comment
@mrjimenez

mrjimenez Jan 15, 2018

Owner

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.

Owner

mrjimenez commented Jan 15, 2018

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.

@v00d00

This comment has been minimized.

Show comment
Hide comment
@v00d00

v00d00 Jan 18, 2018

Contributor

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

Contributor

v00d00 commented Jan 18, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment