On 1/5/10 10:45 AM, Dermot McCluskey wrote: > Evan, > > In that case, shouldn't "errsvc" be added to the __all__ > list in /usr/src/lib/install_utils/__init__.py?
Yes it should be added to the __all__ list and I should be fixing that as part of the fix for bug 13689. > > > > 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. > > > I was referring to "package" in the Python sense - not the > SVr4 sense. Basically, when __init__.py is present, that > directory contains a package. And > import osol_install.errsvc > would not work if __init__.py wasn't there. Correct it would not but since that is there and the proper dependencies are already in place for SUNWinstall-libs (including __init__.py) adding osol_install.errsvc is the correct thing to do. > > And if "errsvc" is not added to the __all__ list, then > import * from osol_install > would not import errsvc. Correct we do need to add errsvc to __all__ so that this works as well. Thanks for pointing out that errsvc was missing from __all__. I'll get this added. -evan > > > - Dermot > > > > On 01/05/10 17:07, Evan Layton wrote: >> 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 >>
