On 1/5/10 9:43 AM, Dermot McCluskey wrote: > 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.
That's why we should be importing osol_install.errsvc not just errsvc. This import does need to be fixed (line 38 in errsvc.c). > > And I was assuming that errsvc is a simple module - not a package. It is a simple module but it's part of an existing package (SUNWinstall-libs) and we need to put it in these same places so it can be used by other parts of caiman. > > > 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 This is the proper place to put this and those below > > /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? This should be correct and this is the standard directory for the install related python code. Thanks, -evan > > > - 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
