ntpq mrulist bug

[Eric S. Raymond]

Hal Murray via devel <devel at ntpsec.org>:
> I know what's going wrong, but I'm not enough of a python geek to see a clean 
> fix.
> 
> The basic idea is that the client sends a request and the server sends back a 
> clump of packets.  The client specifies the max clump size.  What's happening 
> is that at least one packet of the clump is getting lost.  The code is 
> supposed to reduce the clump size when that happens, but that's not working.
> 
> Here is the code outline:
>   mrulist calls doquery to get a clump of data
>     there is an except phrase to reduce the clump size
>   doquery calls sendrequest then getresponse
>   getresponse returns the answer in self.response
>     it also returns None (and maybe falls off the end with no return?)
> 
> I think getresponse needs to return two things.
>   one is the data
>   the second is a flag: none, some, all
> 
> There are lots of raises and excepts in there that I haven't sorted out.
> 
> I think the code should return partial data.  It doesn't.  Or it doesn't get 
> processed.
> 
> What's the right way to structure this?  Should we fix the current code, or 
> make a drastic change?

I don't think I know yet.  I lean towards an incremental fix along the 
lines you describe, but it's also possible that there's a serious design
flaw that merits a rewrite.

Please file an issue and assign it to Ian Breune; he's the maintainer
for the Python parts.  I'll step in if he needs help.

> ----------
> 
> Where is the if for this else?  Can else go with something other than if?
> 
> The code past the else is the normal case.  It gets run if the break doesn't 
> happen.
> 
> This chunk of code is from near the end of getresponse in pylib/packet.py
> 
>             # If we've seen the last fragment, look for holes in the sequence.
>             # If there aren't any, we're done.
>             if seenlastfrag and fragments[0].offset == 0:
>                 for f in range(1, len(fragments)):
>                     if fragments[f-1].end() != fragments[f].offset:
>                         warndbg("Hole in fragment sequence, %d of %d"
>                                 % (f, len(fragments)), 1)
>                         break
>                 else:   <=== this one
>                     tempfraglist = [ntp.poly.polystr(f.extension) \
>                                     for f in fragments]
>                     self.response = ntp.poly.polybytes("".join(tempfraglist))
>                     warndbg("Fragment collection ends. %d bytes "
>                             " in %d fragments"
>                             % (len(self.response), len(fragments)), 1)

That's a Pythonism.  An else clause attached to a for executes only if the for ran
to complewtion without a break.  In this case, the code checks for a hole in the
fragment sequence and sets the response field if there is no hole.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>