Bug#668111: apt: better handling of external redirections
Hi David,
Thanks for taking a look at it.
On Monday 14 May 2012 11:16:49 David Kalnischkies wrote:
> On Mon, Apr 16, 2012 at 5:59 AM, Raphael Geissert <geissert@debian.org>
wrote:
> > Comments?
> >
> > I'd really like to see it included some time soon. Thanks in advance.
>
> Sorry for the delay. We were focused on pushing 0.9 out of the door
> and in to testing which is finally done as of speaking now… *sound of
> relieve*
Yes, that's what I thought.
> I see no big problem with not including it for wheezy so far.
That's great news.
> > Attached version sort of restores the redirection limits. It adds a new
> > setting that limits the number of queues that may exist for a given
> > method that uses a QueueHost mode. Whenever the limit is reached, only
> > one more queue will be created: one without the host name (as in
> > QueueAccess mode).
>
> You have set the limit to 10. Is there some reason for that?
> By default a normal user will have at least three queues (debian,
> security, backports).
> That should be enough and the consequences if not aren't really
> noticeable anyway, i am just interesting in finding out why exactly 10…
>
> Is it because of redirection loop detection (the one in a method is also
> 10)?
At least when using http.d.n that would be: http.d.n, security, and one up
to 5 different mirrors for debian, and up to some other 5 for backports {1}
{1} currently there's no such limit and I'm still trying to make my mind up.
Also, since many backports mirrors also have the main archive, it is
possible that there's some overlap, thus reducing the number of absolute
hosts.
> Either way, this makes it around 100 redirections before a loop is
> detected. (or to be corroct: redirects aren't followed anymore
> regardless of looping) That are not exactly a few and should be quiet
> noticeable for a user, but as it is an error condition anyway it should
> be okay.
Well, that's the worst condition. Usually loops occur within the same
server.
> > Under the normal circumstances of a loop on the same host, the loop
> > will still be caught at the method process. Under a loop of external
> > redirections, it will take a little longer to detect it, but it will
> > eventually be found.
>
> "Longer" just means three times on the same uri instead of two, right?
> (A -> B -> A [A.stop set] -> B [B.stop set] -> A [stop seen] -> failure)
> Shouldn't be a problem, just want to confirm that we are talking about
> the same or if this hints at something i haven't noticed so far.
Yes, that's correct.
> > [patch itself]
> >
> > unsigned long QueueHostLimit;
>
> Such a change would be an abi break, but we don't need that as we could
> use as private struct for which we have already reserved a d-pointer or
> we could just read the value in the only place we accessing it directly.
> For simplicity i have just done the later in the attached patch.
Yeah, I wrote that expecting the change to be accepted for the 0.9
transition. Your way is cleaner too.
> > Queue = Queue->Next;
> >
> >- I->Next->Next = 0;
> >- if (QueueBack == 0)
> >- QueueBack = I->Next;
> >+ delete Tmp;
> >+ if (Tmp == QueueBack)
> >+ QueueBack = Queue;
>
> deleting Tmp and after that comparing it with QueueBack
> looks strange (== like accessing an invalid pointer).
> But this seems like a copy&paste error given that the same code
> is used twice in acquire-method.cc. Just added a small helpermethod
> because of that to take care of dequeue. Patch attached for simplicity.
Ah, yes, I had forgotten about that bit.
> If you are fine with my changes and if i am not running into problems
> with it over the week you should find it included in the next upload.
Just one minor nitpick on the changelog entry, to avoid confusion: my change
to methods/http.cc does prevent the protocol switch now that it is possible
to do so. In previous versions, if apt receives an http redirection to
another protocol, it would still try to resolve it as http.
Cheers,
--
Raphael Geissert - Debian Developer
www.debian.org - get.debian.net
Reply to: