Keith,
> The path this define points to should already be in the Python path .../vendor-packages might be, but .../vendor-packages/osol_install/ isn't. And I was assuming that errsvc is a simple module - not a package. Is there a project-wide standard for where to put Python and Python-C files? Currently, we install these files: /usr/lib/liberrsvc.so.1 /usr/lib/liberrsvc.so -> ./liberrsvc.so.1 - C shared-object lib for accessing errsvc /usr/lib/python2.6/vendor-packages/osol_install/errsvc.py - Python code /usr/lib/python2.6/vendor-packages/osol_install/liberrsvc.py /usr/lib/python2.6/vendor-packages/osol_install/_liberrsvc.so - SWIG generated files for sharing constants between C and Python Maybe osol_install is the wrong directory for errsvc to go into? Who can advise? - Dermot On 01/05/10 15:40, Keith Mitchell wrote: > 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
