Keith, Thanks for reviewing.
This email only addresses your comments for errsvc.c. > General question: What's the advantage/purpose of the use of threading > in this code? Do you mean the code in _start_threads() and _stop_threads()? This was based on similar code in ./lib/libtransfer_pymod/libtransfer.c and reading the API docs, eg: http://docs.activestate.com/activepython/2.5/python/api/threads.html which states "The Python interpreter is not fully thread safe. ... there's a global lock that must be held ... Without the lock, even the simplest operations could cause problems..." Do you have specific concerns about multi-threaded 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) Each public function in this file will only call (at most) one Python method/function, which should be clearly specified in the comments at the start of the function and should also be obvious from the function name (eg C function es_create_err_info calls Python function create_err_info - some others aren't quite so obvious). However, there's no harm in adding more clarity, so I'll expand those comments now. > 61: What's the purpose of hardcoding the path here? Seems like this > would cause maintenance problems and reduce flexibility. The code that uses PY_PATH seems to have been deleted, but I'm checking if that was deliberate or accidental and will get back to you. Basically, before calling Py_Initialize() we need to prepend PY_PATH to PYTHONPATH so Python knows where to find our code at runtime. Is there a better file to put the definition of PY_PATH in? > 110: "UNKNOWN ERROR" - should this be #defined? Yes - I'll fix this. > 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? When users call C functions such as es_get_errors_by_modid(), it creates a new linked list, each element of which contains a PyObject which can be used to get more info about the actual error. es_free_err_info_list is for freeing this *list*, but does not clear or free that actual errors, which are stored within Python. es_free_errors() *does* free the errors by calling a Python function. However, before doing that we have to clean up the C-side references: each error should have one outstanding reference from its initial creation. So, we fetch a list of all the errors, and decrement them - *but* the act of fetching the list increments the refs (and allocates some memory) so we call es_free_err_info_list to tidy this up! Should I put this explanation in the code? > 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. That's really a comment on the Python code in errsvc.py, not errsvc.c, right? (btw - I think we *are* raising exceptions where appropriate on the Python side. Can you elaborate your point in case I'm missing something?) > 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? It's true that many of the functions in this file follow similar templates but I think we've used convenience functions (like _initialize(), _start_threads(), _load_module(), _stop_threads()) as much as makes sense. The Python-calling functions all differ in respect of whether they call a *function* or a *method*, whether or not they pass parameters into Python, what they do with the return value from Python and whether they need to check for raised exceptions. - Dermot On 01/04/10 15:12, Keith Mitchell wrote: > 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 > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
