[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Bug-wget] Implementing draft to update RFC6265

From: Darshit Shah
Subject: Re: [Bug-wget] Implementing draft to update RFC6265
Date: Wed, 27 Jan 2016 21:32:45 +0100

On 27 January 2016 at 20:52, Kushagra Singh
<address@hidden> wrote:
> On Wed, Jan 27, 2016 at 5:06 PM, Tim Ruehsen <address@hidden> wrote:
>> > > What about the '#ifdef HAVE_SSL' ? Don't we need the check always ?
>> Sorry for my irritating text. What I tried to ask/say was "Do we need the
>> #ifdef in cookie_handle_set_cookie() at all ?".
>> And btw, do we need it in parse_set_cookie() ?
> I think it is required in parse_set_cookie(). It does not create a secure
> only cookie in case the connection is insecure. Now this can happen because
> of two reasons, (i) communication over simple HTTP despite wget configured
> with SSL, (ii) wget configured with the "--without-ssl" option. The log
> output in both the cases should be different, right?

I don't see the point of it. Why should it be any different? In fact,
why does the end user, who probably installed a distro-packaged
version of Wget care about the configure options?
Every #ifdef statement you add increases the complexity of the code
since it changes what portion of the code is compiled. As long as the
connection is insecure, Wget refuses to set a secure cookie, period.
Don't overengineer the situation.

>> Darshit said it with clearer words (and I agree with him):
>> "When a user loads a file backed cookie jar, they expect it to work
>> according to the RFC, irrespective of whether the client supports SSL
>> or not. And especially since support for this does not depend on the
>> actual linking of any SSL library, it shouldn't be hard to implement."
> In this case, then can we simply remove the #ifdef check, and and the if
> else statement check whether (scheme == SCHEME_HTTP) and not (scheme !=
> SCHEME_HTTPS), since they would essentially mean the same. This should take
> care of the problem you mention. I have attached a patch with these changes.

Seems okay to me right now.

Please prefer to not move functions around. Adding a prototype to the
top of the file is a better option. Moving a function around like this
causes things like git blame to not work very well.

On a sidenote, I think the find_chains_of_host() method could use some

One is that count_char could use a library function like strchr()
instead of trying to run a pass manually.
Apart from that, I think some parts could use help from Libpsl.
@Tim: When progressing to less specific domains, I think Libpsl could
provide a way to test if we're moving into a new TLD?

There's also the section with a while(1) loop that exits using a
simple condition and a break statement. This is bad programming
practice. We should avoid using such a pattern since it obscures the
actual loop condition. Maybe we can refactor it slightly?

> A question about the way things are done in the Wget project, should I
> attach a patch that should be applied in continuation to the last patch I
> sent, or one generated by all the commits? The patch I have attached is the
> one generated of the last commit only.
You should resend the entire patch again. So that everyone has context
and we can simply apply the final version directly. There is no point
in keeping a history of personal edits on the master branch.
When you have a large change that can be logically split into
different commits, you should have multiple patches for each of the
commit. Think of it this way, once you're done implementing a feature
or a bug fix, what version of your code do you want the rest of the
world to see? The one where you made a hundred stupid errors and later
fixed it, or just the final clean version?
> Kush

Thanking You,
Darshit Shah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]