Hi Dermot,
Dermot McCluskey wrote: > 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? No specific concerns, I was just curious. I'm sufficiently new to C (and Python embedded in C) that I wasn't sure at a glance if the code was doing work in a separate thread, or simply enabling thread "support" so that a multi-threaded Python program can handle the code. It sounds like the latter - thank you for the explanation. > > > > 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. Thanks. > > > > 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? The path this define points to should already be in the Python path (vendor-packages is there by default). By forcing it to the front of the PYTHONPATH, you remove the ability to debug by having someone set the PYTHONPATH, since Python checks in the PYTHONPATH first. e.g., If I build a custom version and put it in /export/home/test-space/osol_install/errsvc.py, I could add /export/home/test-space to my PYTHONPATH, and would expect Python to pick up the custom version. But, if PY_PATH is forced to the front, of my PYTHONPATH, Python won't ever look in my test-space directory. > > > > > 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? That explanation was useful, and should probably be in the code. Thanks! > > > > 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? Right, I was expanding on a comment I mad against the .py file above. > > (btw - I think we *are* raising exceptions where appropriate > on the Python side. Can you elaborate your point in case > I'm missing something?) Yes, there was a single case of using return codes in Python code (see my comments on errsvc.py). > > > > 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. Thanks! I suspected as much. - Keith > > > - 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
