Remove duplicate code between parse_{c,s}method_line

parse_{c,s}method_line share lots of duplicate code. We must find a way to merge the two functions, or hide the duplicate code into functions.

changed milestone to %Tor: 0.3.4.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac duplicate-code in Legacy / Trac easy in Legacy / Trac milestone::Tor: 0.3.4.x-final in Legacy / Trac points::1 in Legacy / Trac priority::medium in Legacy / Trac refactor in Legacy / Trac resolution::implemented in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tor-client in Legacy / Trac type::task in Legacy / Trac labels

Trac:
Keywords: N/A deleted, tor-client added

Trac:
Component: Tor Client to Tor

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: unspecified

Trac:
Points: N/A to 1
Keywords: tor-client deleted, tor-client easy refactor duplicate-code added
Sponsor: N/A to N/A
Severity: N/A to Normal
Reviewer: N/A to N/A

Hey! I am a beginner. Can I work on this?

Trac:
Username: aruna1234

Replying to aruna1234:

Hey! I am a beginner. Can I work on this?

Go for it!

Let us know on IRC if you have any questions :)

Hey,

I would really like to work on this, just wondering if I should go through pluggable transport tor specs first or should I directly approach the issue.

Trac:
Username: fristonio

I think just reading the code should be sufficient here.

If you want to understand the code more, take a look at the right parts of the pluggable transport spec.

Have fun!

Thanks, I will try to make a patch as soon as possible :)

Trac:
Username: fristonio

For this, I was thinking of wrapping up the common functionalities of the two methods into a single function which can then be used by both of them to carry on parsing the line. Is this a good way to go or should I think of some other way to merge them?

Trac:
Username: fristonio

Trac:
Username: fristonio
Cc: N/A to deepeshpathak09@gmail.com

Replying to fristonio:

For this, I was thinking of wrapping up the common functionalities of the two methods into a single function which can then be used by both of them to carry on parsing the line. Is this a good way to go or should I think of some other way to merge them?

Yes indeed fristonio. That's the logic.

Here are the things that I would try to united between those two functions:

a) method_name parsing and validation b) addrport parsing and validation c) transport_new and adding it to mp->transports.

Try to keep it simple :)

Good luck :)

Sorry asn I was a bit busy with my exams.

I have created a patch for this https://github.com/fristonio/tor/tree/ticket-6236 Please have a look and suggest any changes if required.

Trac:
Username: fristonio

Replying to fristonio:

Sorry asn I was a bit busy with my exams.

I have created a patch for this https://github.com/fristonio/tor/tree/ticket-6236 Please have a look and suggest any changes if required.

Hello! Thanks for the code! Concept looks pretty good!

Some simplification suggestions: a) You don't really need to compare proto_method with PROTO_CMETHOD since you control those two things yourself anyway. Instead of proto_method, I'd pass an is_smethod boolean variable to the helper function which would specify whether it's an SMETHOD or CMETHOD line. That way you don't need to do all these strcmps either, and you can just do if (is_smethod) { }. b) You don't really need this proxy_manager thing. You just use it in a log statement, and there you can do:

    log_warn(LD_CONFIG, "%s managed proxy sent us a %s line "
             "with too few arguments.", is_smethod ? "Server" : "Client",
             proto_method);

c) You don't need the else clause in the final logging if block. Since you have already validated on top that proto_method is either CMETHOD or SMETHOD. d) You don't need to pass SMETHOD_MIN_ARGS_COUNT as a function argument. Instead you can set min_args_count inside the helper function based on is_smethod.

Trac:
Status: new to needs_revision

Thanks a lot, those strcmp's were looking a real pain when I was writing them. It was a valuable suggestion from your side. :)

I have updated the branch with the changes you asked, can you have another look https://github.com/fristonio/tor/tree/ticket-6236.

Trac:
Username: fristonio

Replying to fristonio:

Thanks a lot, those strcmp's were looking a real pain when I was writing them. It was a valuable suggestion from your side. :)

I have updated the branch with the changes you asked, can you have another look https://github.com/fristonio/tor/tree/ticket-6236.

Hello, that looks good.

I also pushed a squash commit in branch ticket-6236 in my repo: https://gitweb.torproject.org/user/asn/tor.git

Please check it out, and if you like it, squash it into your branch (so that the final branch only has one commit) and push it out. Then mark this ticket as merge_ready.

Thanks! And props for writing a changes file :)

Thanks for your feedback asn, I have added your patch to the branch. Here is the final patch https://github.com/fristonio/tor/tree/ticket-6236.

Trac:
Username: fristonio

Trac:
Username: fristonio
Status: needs_revision to merge_ready

Trac:
Milestone: Tor: unspecified to Tor: 0.3.4.x-final

This looks correct to me, and I'm glad to see the original code has good test coverage. Merged to master!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 8h

moved from legacy/trac#6236 (moved)

added First Contribution Task labels and removed 1 deleted label

added Client + 1 deleted label and removed 1 deleted label

added Technical Debt label and removed 1 deleted label