Hi Dave,
Comments in-line.
Thank you,
Clay
On Fri, 21 Aug 2009, David Marker wrote:
> I don't think line 104 of libaiscf.py is what you want.
>
> new_service() in libaiscf_instance.c returns "None".
> That makes sense. But that means 104 should be two lines:
>
> super(AISCF,self).new_service(self, service_name) # go create it
> return AIservice(self, service_name) # look it up, may throw KeyError
Good point on the race. Out of curiosity, what would you suggest the
error string to be for if the look up fails? Though it's possible, I'm not
sure what to tell someone when they're making a new service and before I
can return the new service, someone has already deleted it? Perhaps:
KeyError: _("The service %s requested has already been removed from the
system.")%service_name
> A doc string explaining that a KeyError is possible would help too.
> Its understandable that it could throw this since you don't cache SMF
> values (which is good), but that might surprise folks a little.
>
> Otherwise its looking pretty good. Well apart from camelCase ;)
I thought I had followed PEP8 guidelines for the most part with functions
having underscores. Though in the C code to help distinguish what is being
held in a Python object versus a C structure being passed around the
Python objects use cameCase. Is this okay, or would you suggest changes?
> But then you got the constant LHS test from my examples which
> Jean doesn't like (I use it because the compiler won't allow a
> constant expression on LHS of *assignment* which is a class
> of bug I seem to create).
Yeah it did help my writing and like the style for catching errant
assignments. I tend to typo my equalities and assignments as well.
> -dvd
>
>
> On Aug 21, 2009, at 12:07 AM, Clay Baenziger wrote:
>
>> Hi Jean,
>> Thank you for looking this thing over! My comments are in-line.
>>
>> New webrev:
>>
>> Full:
>> http://cr.opensolaris.org/~clayb/10740/webrev3
>>
>> Diff from webrev 2:
>> http://cr.opensolaris.org/~clayb/10740/webrev_2_3_diff/
>>
>> Thank you,
>> Clay
>>
>> On Thu, 20 Aug 2009, jeanm wrote:
>>
>>> Nit: libaiscf.py line 64: service is misspelled.
>>
>> Fixed
>>
>>> libaiscf_pymod/Makefile: copyright date should be 2009
>>
>> Grr, I thought I'd gotten that but I've ensured it's changed now...
>>
>>> General: Please add header block comments for all new methods. Some are
>>> there, some aren't.
>>
>> I added header blocks to every function now I believe
>>
>>> libaiscf_instance.c line 411-412: Why the empty if?
>>
>> Good question, it's now set to if (ret != AI_SUCCESS) with no else.
>>
>>> libaiscf_service.c line201 and 209: Any reason why you did if (0 !=
>>> function()) and not if (function != 0) ?
>>> Most people will be more used to seeing the later which makes it easier to
>>> read.
>>
>> Yes, it's a rather long block that we're checking though. We're checking
>> the return, so it's easier for one to see what we're checking it against
>> than burrying the != 0 down below.
>>
>>> Jean
>>>
>>> Clay Baenziger wrote:
>>>> Hi all,
>>>> Per some initial feedback from Jean and Dave I've come to round two.
>>>> Here are a few fixes for Python objects which had reference counts which
>>>> were wrong (memory leaks) and I've added support for specifying the FMRI
>>>> for AISCF() objects bringing them closer to being fully general.
>>>> The diffs can be found at:
>>>> http://cr.opensolaris.org/~clayb/10740/webrev_1_2_diff/
>>>> Or the full webrev at:
>>>> http://cr.opensolaris.org/~clayb/10740/webrev2/
>>>>
>>>> Thank you,
>>>> Clay
>>>> On Wed, 19 Aug 2009, Clay Baenziger wrote:
>>>>> Hi all,
>>>>> I believe the code is stable, clean, builds correctly and ready for a
>>>>> full code review. I've implemented the last necessary for the library.
>>>>> Now, one can view an instance's state (online, offline, maintenance,
>>>>> etc.) and one can change the state too. For example:
>>>>>>>> libaiscf.AISCF().state
>>>>> 'online'
>>>>>>>> libaiscf.AISCF().state="DISABLE"
>>>>> [wait a bit (~5 sec), or it'll return online still]
>>>>>>>> libaiscf.AISCF().state
>>>>> 'offline'
>>>>> Webrev now at:
>>>>> http://cr.opensolaris.org/~clayb/10740/webrev/
>>>>>
>>>>> Thank you,
>>>>> Clay
>>>>> On Tue, 18 Aug 2009, Clay Baenziger wrote:
>>>>>> Hi all,
>>>>>> I've written a Python<->C bridge for libaiscf. It provides a few ways
>>>>>> to interact with SMF. I'm looking for some folks to look the code over
>>>>>> for memory management issues, for any potential SMF issues or any other
>>>>>> issues others might see. This code has not be whacked to be c-style
>>>>>> happy, so don't worry about that unless you really want.
>>>>>> I am having issues with the delete section of
>>>>>> AIservice_set_subscript(), if anyone has any suggestions what might be
>>>>>> happening (dbx stack trace included). This is triggered when one tries
>>>>>> to delete a property via:
>>>>>> del(libaiscf.AIservice(libaiscf.AISCF(),"test")['status'])
>>>>>>
>>>>>> Thank you,
>>>>>> Clay
>>>>>> Webrev is at:
>>>>>> http://cr.opensolaris.org/~clayb/10740/webrev_prelim1/
>>>>>> How the module works:
>>>>>> ---------------------
>>>>>> To load the module, one runs
>>>>>> import libaiscf
>>>>>> Then to create an SMF instance object, one can run
>>>>>> (for svc:/system/install/server:default)
>>>>>> instance=libaiscf.AISCF()
>>>>>> (for svc:/system/install/server:someThingElse)
>>>>>> instance=libaiscf.AISCF("someThingElse")
>>>>> This has changed to:
>>>>> instance=libaiscf.AISCF(instance="someThingElse")
>>>>>> Further, to create an AI service object, one runs:
>>>>> For clarification, this is just an SMF property group representation
>>>>>> service=libaiscf.AIservice(instance,"serviceName")
>>>>>> To see a property key under the service, one can do:
>>>>>> service['boot-dir'] (which returns the string value or a KeyError as a
>>>>>> dictionary would if the key doesn't exist)
>>>>>> To set or change a property under the service, one can do:
>>>>>> serivce['boot-dir']="/var/ai/service1_image"
>>>>>> All actions query SMF and no data is cached in case something changes
>>>>>> under the consumer. All actions are handed to SMF when executed. Other
>>>>>> functions implemented can be read in the PyDoc output for the module,
>>>>>> here attached.
>>>
>>>
>
