Hi Evan (and the rest of the Error Service project team), I took a good look at errsvc.py, and a briefer look at the C files (except for the test_*.c files). My comments are below. Looks pretty good!
- Keith errsvc.py: General nit: There are some extra spaces around parens in this file (e.g. at lines 38, 57, 110, etc.) 40-42: Can we define this list as a constant somewhere? 49-55: Is there any reason to not use direct attribute access? 57: Why the use of return codes in Python code? 65: Why this syntax? If this list is expected to grow later, can we define it as a constant of the class? 88: Performance is likely not an issue here, but in general, string concatenation is much more efficient if you create a list of strings, and then use "\n".join(<list>) or something similar. 110-118: Could this be incorporated into the ErrorInfo.__init__? Or, is it a valid scenario to create an ErrorInfo independently from the list? 133: Since we control the adding of errors to the list, would it be easier to simply store separate lists for each type? Or have the global __errors be a dictionary, where the key is error_type and the value is a list of errors of that type? It seems that conglomerating lists together when the full list of all errors is needed would be simpler than picking apart the list to get errors of a certain type. 143: Same question. Since we control adding items to __errors via the create_error_info method, it seems like managing multiple lists (or dictionaries, as appropriate) would be relatively simple. 157: The second check (errs > 0) is redundant - "if errs:" only returns True if a list is non-empty. 158: Nit: "for err in errs:" 159: Nit: "print err" is sufficient. (And, using str(err) instead of err.__str__() is generally more common) 178: Nit: integer -> string 163-185: Can you use the Python built-in isinstance instead? unittesting.py: General: The error cases should be tested too (e.g., calling set_error_data with an invalid type, or with a value that mismatches the type, or instantiating an ErrorInfo with an invalid type). 31-32: Nit: platform.processor() is simple enough to make a call to, to determine sparc or i386. 41: Nit: Instead of sys.exit(1), re-raise the exception (or raise a new one). Calling sys.exit(1) prevents, for example, writing a meta-module that runs a series of unittests (if the import of this module failed, and a reasonable exception is raised, the managing module/script can catch it and move on to the next set of tests - if sys.exit(1) is called, the rest of the script wouldn't get run). 49: Nit: Call it "self.error_types", since it's a list with more than one type. 52: Why store this value? It's easily retrievable with a call to len() 57, 67, 72: Use "for error_type in self.error_types" instead of creating the arbitrary index variable c. It's rare to need to know the index of an item in a list in Python (and when you do, use of the enumerate() function is the best way to track it when looping - line 116 is a good example: "for idx, item in enumerate(self.error_types):") 66, 96, 115: This should be in a tearDown method. 140: Should this be a liberrsvc constant? errsvc.c: General question: What's the advantage/purpose of the use of threading in this code? General note: There are a few comments that are just "Call Python" - it would help if the comment were expanded to indicate what Python code is called (especially since the Python method called can get obscured by the fact that it takes quite a few lines of C code to get to the point of being able to call said method) 61: What's the purpose of hardcoding the path here? Seems like this would cause maintenance problems and reduce flexibility. 110: "UNKNOWN ERROR" - should this be #defined? 566-571: I don't quite understand the comment. It seems like calling the clear_error_list function should take care of clearing out the error list. Where are the extra references coming from that need to be decremented here, instead of by whichever portion of the code grabbed hold of the extra reference in the first place? 683: As mentioned above, Python code should raise errors, and not return error codes. Reserve return values for useful data, and check for exceptions if attempting to handle an error condition. 1030-1078: This bit of code looks very similar to blocks of code in other functions. Is it possible to combine somehow for the purpose of simplifying, or are they different enough that it's not possible to do such a thing in C? liberrsvc.h:60: Nit: _LIBERREVC_H -> _LIBERRSVC_H liberrsvc_defs.h: 38-40, 41-42: Nit: Make these into single block comments so they're more readable. prototype_com: General: The osol_install directory seems to be getting a little cluttered, perhaps we need a 'utils' directory for things like the error service, the future logging service, and so on? Evan Layton wrote: > The CUD Error Service project team is requesting a code review of > the project gate. > > The code implementation is complete and unit testing has been done. > The official testing is in progress but is not yet complete. We will > be providing updated incremental webrevs as code review comments and > bug fixes come in. > > The webrev is available at: > http://cr.opensolaris.org/~evanl/CUD_errsvc > > With the holiday coming up it appears that most people will be out > of town for a while so we'll be giving extra time for folks to look > at this. We'd like to have all comments back by January 15, 2010. > > Thanks! > > -evan > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
