Skip to content
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

Try resolve #1175 #1223

Closed
wants to merge 1 commit into from
Closed

Conversation

pavel-pimenov
Copy link
Contributor

 - New crash dump https://drdump.com/Problem.aspx?ProblemID=241665&Login=Guest
 - Remove raw pointer and static std::string service_type;
@codecov-io
Copy link

codecov-io commented Oct 18, 2016

Current coverage is 70.97% (diff: 77.77%)

Merging #1223 into master will decrease coverage by 0.13%

@@             master      #1223   diff @@
==========================================
  Files           390        390          
  Lines         58181      58182     +1   
  Methods        5728       5728          
  Messages          0          0          
  Branches       8779       8779          
==========================================
- Hits          41369      41294    -75   
- Misses        12443      12526    +83   
+ Partials       4369       4362     -7   

Powered by Codecov. Last update 85fe066...f516e47

@@ -225,7 +225,7 @@ int upnp::add_mapping(portmap_protocol const p, int const external_port
m.external_port = external_port;
m.local_port = local_port;

if (d.service_namespace) update_map(d, mapping_index);
if (!d.service_namespace.empty()) update_map(d, mapping_index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not

if (d.service_namespace && !d.service_namespace.empty()) update_map(d, mapping_index);

and avoid a possible NPE?

The same comment applies everywhere else below, and since it happens so much perhaps I'd add a private utility function to not repeat yourself all over

bool valid_service_namespace(rootdevice &d) {
    return d.service_namespace && !d.service_namespace.empty();
}

and then have

if (valid_service_namespace(d)) update_map(d, mapping_index);

...

TORRENT_ASSERT(valid_service_namespace(d));

...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service_namespace -> std::string
if(string()) - impossible

int main()
{
std::string name;
if(name) {
}
}

In function 'int main()': 8:10: error: could not convert 'name' from 'std::string {aka std::basic_string}' to 'bool'

@arvidn
Copy link
Owner

arvidn commented Oct 19, 2016

First of all, this fix should be made against RC_1_1, that's the version you're seeing this problem in, right?
Second; the existing code is clearly broken, I don't understand how it could get so screwed up. However, I'm tempted to fix it by restoring it instead of entirely switching to a std::string approach.
The original code (as far as I can tell from the intention of the existing code at least) meant for service_namespace to point to a string literal. There are only 3 choices of what this string can be, so that kind of makes sense. Alternatively, we could make it and index and have a lookup table for the actual namespace name.

@arvidn
Copy link
Owner

arvidn commented Oct 19, 2016

going back to RC_1_0 though, it doesn't look like this was ever right. going with std::string may be the better option

@arvidn
Copy link
Owner

arvidn commented Oct 19, 2016

this patch looks good to me. would you mind submitting it against RC_1_1?

pavel-pimenov added a commit to pavel-pimenov/libtorrent that referenced this pull request Oct 19, 2016
arvidn pushed a commit that referenced this pull request Oct 19, 2016
@arvidn
Copy link
Owner

arvidn commented Oct 20, 2016

this has been merged into master from RC_1_1 now

@arvidn arvidn closed this Oct 20, 2016
@pavel-pimenov pavel-pimenov deleted the issue-1175 branch March 31, 2017 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants