On 03/ 6/12 05:50 AM, Shawn Walker wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/poisoned-cache/poisoned-cache-webrev/
Thanks for taking a look,
src/modules/client/transport/repo.py:
line 1034: it should set header["Pragma"] = "no-cache" as well;
that's also true for line 513; necessary for HTTP 1.0 caches [1]
Good point - I've fixed both of those.
lines 213, 1037, 1752: need extra newline
For 213, it's the same class - other places where we switch from
defining normal vs. static methods don't seem to do that. I've fixed
the others though.
lines 1750, 2019: I'd make these docstrings the same and something
like: """Pointless to attempt refetch of corrupt content for
this protocol."""
Sounds good.
src/modules/client/transport/transport.py:
line 917: this comment is subtly different for each retry case
I've fixed that.
lines 918-926: so the problems I see with this approach are (and it
applies to most of the other cases):
* it relies on the number of retries left being sufficient
to retrieve the content (i.e. retry_count) that's probably ok,
but there should be a comment
Yes, I realised that at the time, and sort of threw my hands up in the
air at what to do in the face of a several failed attempts, followed by
one attempt which passes corrupt content... I've added comments.
* if this is the last retry already, then the failure
won't be added to failedreqs / failures (oops), a simple
fix for that would be to track the number of retries and
skip the redownload if retries == retry_count
Good point - thanks. Yes that seems reasonable.
* i'm not sure why it's doing "errlist.append(e)" as
errlist will be re-assigned at the next iteration of the loop
and entries in flist are sufficient to prevent a successful
return
Oops.
src/tests/cli/t_pkg_install.py:
lines 784-788: instead of this, you can grab the fmri from the list
that pkgsend_bulk returns on line 755
Great. I still need to grab the version component of the FMRI in order
to quote it properly, but that's still nicer than parsing pkg info
output, thanks.
lines 790-791: actually, simply doing a forced image-create again
doesn't clear the content, but our image_create() routine
automatically calls the unit test image_destroy() method
Sure - I'll add a comment.
line 800: s/"w"/"wb"/
Fixed.
I've got an incremental, and new webrev at:
https://cr.opensolaris.org/action/browse/pkg/timf/poisoned-cache-v2/
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
