Opened 6 years ago

Closed 6 months ago

#6236 closed task (implemented)

Remove duplicate code between parse_{c,s}method_line

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client easy refactor duplicate-code
Cc: deepeshpathak09@… Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (21)

comment:1 Changed 6 years ago by nickm

Keywords: tor-client added

comment:2 Changed 6 years ago by nickm

Component: Tor ClientTor

comment:3 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: unspecified

comment:4 Changed 16 months ago by nickm

Keywords: easy refactor duplicate-code added
Points: 1
Severity: Normal

comment:5 Changed 10 months ago by aruna1234

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

comment:6 in reply to:  5 Changed 10 months ago by asn

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

comment:7 Changed 7 months ago by fristonio

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.

comment:8 Changed 7 months ago by asn

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!

Last edited 7 months ago by asn (previous) (diff)

comment:9 Changed 7 months ago by fristonio

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

comment:10 Changed 7 months ago by 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?

comment:11 Changed 7 months ago by fristonio

Cc: deepeshpathak09@… added

comment:12 in reply to:  10 Changed 7 months ago by asn

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

comment:13 Changed 7 months ago by 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.

comment:14 in reply to:  13 Changed 7 months ago by asn

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.

comment:15 Changed 7 months ago by asn

Status: newneeds_revision

comment:16 Changed 7 months ago by 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.

comment:17 in reply to:  16 Changed 7 months ago by asn

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

comment:18 Changed 7 months ago by fristonio

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.

comment:19 Changed 7 months ago by fristonio

Status: needs_revisionmerge_ready

comment:20 Changed 7 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.4.x-final

comment:21 Changed 6 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

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

Note: See TracTickets for help on using tickets.