Skip to content
This repository has been archived by the owner on Dec 8, 2023. It is now read-only.

Fix serviceList parser #45

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Fix serviceList parser #45

merged 1 commit into from
Apr 7, 2020

Conversation

nbaksalyar
Copy link
Contributor

Not all routers return a <serviceList> element in the root of the IGD response, so the parser terminated too early (before trying to look for a list of services in <deviceList>).

This PR fixes the bug and adds a new test case for a device with such a response.

Not all routers return a <serviceList> element in the root of the IGD response,
so the parser terminated too early (before trying to look for a list of services in <deviceList>)
@sbstp
Copy link
Owner

sbstp commented Apr 6, 2020

Unfortunately my ISP provided router has broken UPNP support, so I can't test it. I'm gonna try to trust you on this.

@nbaksalyar
Copy link
Contributor Author

Yeah, unfortunately routers show very different behaviour and it's very hard to test rust-igd in the wild. :( Probably the implementation logic can be borrowed from miniupnpc? I think it should be a fairly well tested library that can be relied upon, and it worked for me in the majority of cases.

@sbstp
Copy link
Owner

sbstp commented Apr 6, 2020

Hey. So I dusted off an old router I had laying around and I tried it with that router, and it didn't work with that old router. And it used to work on it!

So I started thinking, "this used to work well on my dual boot machine!". And it dawned on me that I am now using VirtualBox instead of dual booting for development. And my virtual machine was configured in NAT mode instead of bridge mode. So it was broadcasting to some stupid VirtualBox virtual NAT instead of my real router...

So I was able to test your branch by switching the VM to bridged mode. Everything looks good, thank you.

@nbaksalyar
Copy link
Contributor Author

Cool, thank you very much for testing & reviewing this!

@sbstp
Copy link
Owner

sbstp commented Apr 6, 2020

As far as the quality of this library goes, it's probably not up to par with miniupnpc. This project started as just a simple thing for me to get my router's external ip, and then people asked to add more methods to add port mappings, remove port mappings, etc. and it grew organically.

I haven't had an actual use for this library in years, I'm sort of just maintaining it when something pops up. If someone wanted to prop it up and make it more standards compliant I'd happily transfer ownership, or just merge pull requests. I personally don't have the time or energy to give it more love at the moment.

@sbstp sbstp merged commit 544d545 into sbstp:master Apr 7, 2020
@sbstp
Copy link
Owner

sbstp commented Apr 7, 2020

I'll publish a new release, version 0.10.1

@sbstp
Copy link
Owner

sbstp commented Apr 7, 2020

It's out, thanks again!

@nbaksalyar nbaksalyar deleted the fix-parser branch April 7, 2020 21:04
@nbaksalyar
Copy link
Contributor Author

If someone wanted to prop it up and make it more standards compliant I'd happily transfer ownership, or just merge pull requests. I personally don't have the time or energy to give it more love at the moment.

Hey @sbstp, as I use this library for P2P-related projects a lot, I'd love to help with maintenance!
My email is in profile. :)

@sbstp
Copy link
Owner

sbstp commented Apr 7, 2020

For now I'll keep merging and testing what you send me. Eventually we can look at transferring it to you or MaidSafe, what ever you prefer! I know MaidSafe has been the primary user of this library for quite a while!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants