On 12/06/12 21:29, Tim Foster wrote:
Hi there,
I've got a set of changes here for code review, if anyone has time:
https://cr.opensolaris.org/action/browse/pkg/timf/pkgrepo-verify/pkgrepo-verify-webrev/
----------------------------------------
src/man/pkgrepo.1:
line 781: suggest:
Verifies the following attributes of the package repository
contents are correct:
line 815: s/over/using/
line 881: This seems a little too vague.
----------------------------------------
src/modules/client/transport/repo.py:
line 207, 212: I know it seems silly, but I'd prefer we prefix the
function names with something like 'publish_' to be consistent and avoid
potential (although unlikely) namespace conflicts.
line 1039, 1046: s/IO/I\/O/
----------------------------------------
src/modules/server/repository.py:
line 268: This implies that versions other than 4 are supported,
but the rest of the code clearly limits support to version 4.
I know that in the future this may change, but I don't think
the message should account for that now.
line 270, 304: two newlines between classes
line 1877: why not RepositoryUnsupportedOperationError() ?
lines 1853-2403: I don't think that verify() should ever use a
progress tracker to provide information about the issues found.
Instead, it should either yield each chunk of information just as
Image.verify() does or it should accept a callback function that the
consumer can provide to accept the information and let the client handle
the output. In particular, I don't like how the formatting of the
messages has been encoded into the verify() routine itself because of
the current approach. Also, to greatly improve the readability /
maintainability of this function, I would not use nested function
definitions for each piece of logic. I would instead make each one a
private function like 'def __verify_yield_error' or 'def
__verify_get_hashes'. Read on below for additional suggestions.
line 1855: extra newline
lines 1895-1898: I'm not thrilled with dummy FMRIs for these cases.
Is there a reason we have to output FMRIs and can't just use blank
lines or something like "Unknown Package" "Repository" ?
line 1966: did you intentionally avoid 'signature' actions? I also
think we should avoid hardcoding 'file' and 'license' here and instead
look for 'has_payload' or the 'hash' attribute so that if we add
additional actions that can have payloads in the future, we'll get
those. In fact, I think the release notes stuff Bart added may work
this way...
lines 1974-2025: Per my earlier comment, I would yield these back to
the caller as a simple list of strings without the formatting. If
desired, you could do it as a tuple of the form (error_summary,
error_details) where error_summary is something like 'Missing file:
4bca78af3bac' and error_details are the rest of the text. I would
expect the consumer to then decide how to indent or otherwise display
the resulting text. You also shouldn't pre-wrap the output. Let the
caller use the Python textwrap module if they want to wrap the output.
I realise this is all more work, but eventually I'd like to make it
possible to trigger verify / fix from the web interface and the current
way this is being done makes that difficult at best.
lines 2249-2395: You might consider also placing this into a private
function so you can improve the readability a bit; just wrapper the call
to it with the try/except you have now.
line 2403: seems like this should be before the call to rmtree;
if the rmtree fails we won't unlock, but maybe that's the desired
intent?
line 2411: s/referred/referred to/ ?
lines 2411-2414: Doesn't this mean that files can be orphaned?
I admit it's rather expensive to go hunt down every package that's
referencing the unquarantined files, so perhaps we should just punt
on this and add it to 'pkgrepo cleanup' later. (The assumption
being that 'pkgrepo cleanup' would remove orphaned content.)
line 3536: 'given packages' ? Seems like it should be:
Verifies that repository contents matches expected state for all
or specified publishers.
line 3577: Why re-raise UnqualifiedFMRIError here? That doesn't seem
right. I think the normal exception should be sufficient.
----------------------------------------
src/pkgrepo.py:
lines 158-160: These seem incomplete
line 1240: s/verify/verification/
line 1247: drop the 'else:' de-indent the following line
line 1252: s/index,/index/ ?
line 1249, 1323: two newlines between functions
----------------------------------------
src/tests/cli/t_pkgrepo.py:
line 1748: s/0755/misc.PKG_DIR_MODE/
line 1750: s/0644/misc.PKG_FILE_MODE/
----------------------------------------
src/tests/pkg5unittest.py:
lines 2129-2136, 2552-2554: The other way to do this is as follows:
def get_su_wrap_user(uid_gid=False):
import operator
for u in ["noaccess", "nobody"]:
try:
pw = pwd.getpwnam(u)
if uid_gid:
return operator.attrgetter('pw_uid', 'pw_gid')(pw)
return u
except (KeyError, NameError):
pass
raise RuntimeError("Unable to determine user for su.")
...
if su_wrap:
os.chown(f_path, *get_su_wrap_user())
That's mildly improved since you get a tuple of return values.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
