hey danek, i've addressed all your comments but i don't have an updated webrev to send out just yet. i'll do that shortly.
i have replies to some of your comments below (i elided all the comments that i simply addressed). thanks for looking at this, ed On Mon, Jun 11, 2012 at 03:49:10PM -0700, Danek Duvall wrote: > Edward Pilatowicz wrote: > > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.pli.cr1/webrev/ > > client.py: > > - line 1614: from here forward, you never remove this temporary file if a > failure occurs. I'm also a bit surprised that the actual save() isn't > in a try, even though close() is. Is fdopen() a context manager (ie, > can you use with fobj as os.fdopen() ...." > i've moved the save() call into the "try:" block. wrt removing the plan file, we don't remove them until we get to the execution stage. this means if we get interrupted we leave these files around. it's the same behavior we have today when we save the files from within the imageplan.py. this is covered by: 7140602 pkg operations don't remove temporary planning files 7139816 need improved linked image temporary file management that said, i just realized i can improve this. __api_plan_save() doesn't get called unless we're going to execute a plan. this means that we have a lock on the image and no-one else can be updating it. that means that the plan file doesn't have to have a unique extension anymore. by removing the unique extension component, even if we get interrupted and leave the file around, it'll just get overwritten during the next recursive operation. (so at most we'll only have one leaked file.) also, since people probably have old temporary planning files lying around i also added code to check for them and clean them up if they're present. > generic.py: > > - line 122, 128: what does the "j" denote? > j is for json je_state == json encoder state, used by json_encode() jd_state == json decoder state, used by json_decode() > - line 720: why move removed_users to the plan description? It's part of > the plan; shouldn't the description be a little less central? Or ... > what's the rationale for putting one object into the plan description > vs keeping it in the imageplan? > any part of the plan that is computed during the api planning stage must be saved in the PlanDescription object. so for example, if removed_users wasn't moved into the PlanDescription, then when we re-loaded a plan from disk and went to execute it, we wouldn't remove any users (unless we went back over all the actions and re-computed the set of users that need to be removed). > client/api.py: > > - line 386: I would expect this to be a decorator, so that you'd have, > for instance: > > @__activity_locked_cb > def set_alt_repos(): > .... > > where the content of set_alt_repos() was what's in > __set_img_alt_sources() right now. > i was unaware of decorators. thanks for pointing this out. i've created _activity_lock_decorator(), and in the case of load_plan() and linked_publisher_check(), i was able to remove the associated __load_plan() and __linked_publisher_check() methods. but i still have to keep set_alt_repos() and __set_img_alt_sources() as separate methods because set_alt_repos() is a public api interface, but there are still internal callers of __set_img_alt_sources() that are already holding locks. > - line 3916 et al: while you don't need to create a new ImagePlan object > here, you can still keep the shortening to ip. -- fewer changes, and > better readability (IMO, at least where you use it twice; less so in > __calc_frozen()). > (i think this comment applies to image.py) i'm not sure i understand the comment. do you want me to change: import pkg.client.imageplan as imageplan to: import pkg.client.imageplan as ip and then rename any variables called "ip" to "imageplan"? > imageconfig.py: > > - Are there cases where these unicode-to-string conversions would fail? > nope. in all cases, the values that were saved into the config file were originally strings. (i talked with shawn about this specific point to make sure i'm not making any invalid assumptions.) > imageplan.py: > > - line 186: the comment expression isn't particularly useful here, and > it'll hide the rest of the assertion. Either ditch it or enhance it. > i can't match the line numbers up, but i'm assume you were referring to the following lame comment (that i've removed): # can't skip preexecute since we already preexecuted it > pkgremote.py: > > - line 166: why use real files? > what would you recommend in place of real files? i don't want to use pipes because unless i create threads to constantly read and drain data from the pipes, clients could block while writing output to stdout. > progress.py: > > - line 138: why would these happen, and why is it safe to ignore them? > i was ignoring errors because the progress pipe is informational. currently it just drives the child activity spinner in the parent (if the parent is using the fancy cli progress tracker). but given that an error here would indicate a problem in the parent, i've remove the blanket error handling. > misc2.py: > > - I think it's fine that you developed this module to be lint-clean, but > I don't want it shipped as a separate file. Please fold this back into > misc.py once the code reviews are done. > i already folded it back in in my last webrev. > - line 258: what does this mean for unicode action attribute values? > shawn corrected me about this point, simplejson does support unicode. (i went back and re-tested this to verify.) i've removed this comment and assert(). > - line 550: shouldn't need frozenset here. > why don't i need frozenset? a frozenset is not the same as a set: ---8<--- >>> isinstance(frozenset(), set) False ---8<--- > - line 624: no defaultdict? Similarly, on 638: no tuple or set? > no. json_validate() verifies that the data passed into it is directly json encodable, and json doesn't have defaultdict, tuple, or set types. (this function is called on the output of json_encode().) i've updated the comment in this function to say: """Validate that a named piece of data can be represented in json and that the data can be passed directly to json.dump(). If the data can't be represented as json we'll trigger an assert. > pipeutils.py: > > - Why use real files? > i've updated the docstring to include: This is done to help ensure that processes don't block while writing to these pipes (otherwise consumers of these interfaces would have to create threads to constantly drain data from these pipes to prevent clients from blocking). > - line 218: the fd you send won't be pointing to the end of the file? > no. because sendfd/recvfd operate at the vnode level (not the file_t level, which is where current file offsets are stored). ie, when someone receives a fd, they get a new file_t with an offset of 0. > - line 359: should there be a close() here? What __del__() and close() > do in this class are the reverse of what they do in _PipedTransport(); > is that on purpose? > i've updated these all to be consistent. > prstart.c: > > - We get memory information from python by simply reading psinfo. Any > reason we couldn't do the same for prstart? > yeah. i didn't realize we already getting information from psinfo. i also didn't know about struck.unpack() when i wrote prstart.c. :) i've nuked prstart.c, and i created a class called ProcFS info in misc.py that provides easy access to psinfo data. (i also wrote a test case for it and out_of_memory() since i modified that.) _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
