Evan,
In that case, shouldn't "errsvc" be added to the __all__
list in /usr/src/lib/install_utils/__init__.py?
> 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.
And if "errsvc" is not added to the __all__ list, then
import * from osol_install
would not import errsvc.
- 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
>
