ntpq mrulist bug
Eric S. Raymond
esr at thyrsus.com
Wed Nov 20 18:24:13 UTC 2019
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
> 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
> 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
> 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.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)
> 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>
More information about the devel