On Oct 1, 2009, at 4:57 PM, Clay Baenziger <clayb at sun.com> wrote:
> Hi Evan,
> Thank you for the review. Comments in-line.
> Thank you,
> Clay
>
> On Mon, 28 Sep 2009, Evan Layton wrote:
>
>> Hi Clay,
>>
>> Just a few more comments on the "C" code only...
>>
>> installadm.c
>>
>> line 899 - This needs to be changed as we discussed earlier today so
>> that we give a bit more description about what this function does.
>> Also we need to add a bit more description on what type of thing
>> SERVICE_DELETE_SCRIPT does. for example maybe saying something like
>> "the remaining clean-up is done by SERVICE_DELETE_SCRIPT" ( if
>> that's at all accurate... ;-) ) would help.
>
> I've made it clear we're calling the SERVICE_DELETE_SCRIPT but I
> still really don't want to enumerate what is done there. The
> engineer necessarily needs to read the SERVICE_DELETE_SCRIPT to know
> what it does
Which is why I suggested the comment that I did. What you do know at
this point is what this function does and the comment should reflect
this. The comment also needs to be descriptive of what
SERVICE_DELETE_SCRIPT does. I'm not suggesting that everyting it does
should be enumerated but we definitely need more then just "everything
else". All this being said I think either what I suggested or what
you've suggested below is probably adiquate.
> /*
> * do_delete_service:
> * Simply call SERVICE_DELETE_SCRIPT
> * All service deletion is handled and done in the
> SERVICE_DELETE_SCRIPT
> */
>
>> line 1168 The comment here is a bit misleading. What this really
>> does here is if there are no service instances with a state set to
>> "on" the we put the smf service into maintenance mode. We should
>> state that this is what is happening instead of saying it disables
>> something.
>
> Yes, I was getting my metaphors mixed up. I've changed the comment to:
> /* if no longer needed, put install instance into maintenance */
Put should probably be puts.
>
>> Also this points out the fact that the state can either be on or
>> off but there can be a third state where we've temporarily disabled
>> a service instance but still want it to start up the next time the
>> SMF service is started. This transient state is not dealt with in
>> here but is definitely outside the scope of this fix and I believe
>> this is now covered in bug 11604 that you filed earlier.
>
> I've filed bug 11604 for this indeed.
>
Sounds good, thanks for filing this.
>> ai_utils.c
>> line 166 - It might be helpful to state here why no data out of the
>> handle is needed here instead of just stating that we don't need
>> anything out of the handle here. Maybe something along the lines of
>> saying that the handle is not needed for this call since we not
>> destroying the property stored in the handle.
>
> I've expanded the comment to explain we were loading the property in
> to the SCF handle
>
> /*
> * No data is needed out of the property handle, we only need to load
> * the property in the SCF handle which was done by getting the PG
> */
>
Sounds good.
>> libaiscf_backend.c
>> line 82 - This isn't really "bound" to our handle but is stored in
>> the handle. It may make more sense to say "Allocate and initialize
>> a property group and store it in the handle".
>>
>> line 87 - Same thing but with the instance.
>
> I've made lines 71, 76, 82 all the same as 87 but took out
> initializes since there's no initialization happening. So, now for
> example:
>
> "Allocates a new scf_scope_t stored in the handle."
>
Sounds fine.
>> line 90 - maybe "Make sure we have what's needed to run SMF" ?
>
> Since we aren't really running SMF, I've changed to:
> "Make sure we have everything to communicate with SMF"
>
Sounds fine.
>> libaiscf_instance.c
>> line 264, 316 - Closure should be closure so that it matches the
>> actual parameter.
>
> Done
>
>> libaiscf_service.c
>> line 259 - should be "static PyObject *AIservice_print(PyObject
>> *self) {"
>> Also this doesn't actually print anything out does it? Seems
>> like a confusing name for this function. Not a big deal I
>> guess but it could cause confusion. (sorry I didn't notice
>> this before...)
>
> No problem, you're right this isn't the right verb. I've changed to
> AIservice_Str (to follow PyObject_Str).
>
Ship it ;-)
>> -evan
>>
>> Clay Baenziger wrote:
>>> Hi again all,
>>> I got a way to get webrev to work for me to do a differential
>>> including delete_service.py (still known as delete-service.py in
>>> the differentail webrev). See:
>>> http://cr.opensolaris.org/~clayb/delete_service/webrev2.diff2/
>>>
>>> Thank you,
>>> Clay
>>> On Wed, 23 Sep 2009, Clay Baenziger wrote:
>>>> Hi again all,
>>>> Well the addition of a thousand lines of code and 50+ pages of
>>>> comments I think I've got this re-spun for everyone's enjoyment,
>>>> please see:
>>>> http://cr.opensolaris.org/~clayb/delete_service/webrev2.diff/
>>>> (Unfortunately I can't get webrev to track that delete-service.py
>>>> became delete_service.py (so that it can be imported by
>>>> delete_client))
>>>> Or full webrev:
>>>> http://cr.opensolaris.org/~clayb/delete_service/webrev2/
>>>> The bug list has grown to include:
>>>> 4526 delete-service is not deleting service as described in
>>>> section 4.3.2
>>>> ai_design_doc
>>>> 6587 delete-service shouldn't remove the source image if
>>>> there's other
>>>> services actives 'linked' to the same source image
>>>> 8666 create-service: prints out SMF messages no matter what's
>>>> going on
>>>> 8773 create-service followed quickly by delete-service hangs
>>>> 10740 Need way to interact with SMF from Python for installadm
>>>> components
>>>> in Python
>>>> 11292 delete-client: should remove SPARC clients too
>>>> 11486 delete-service/delete-client: should check inetd.conf
>>>> for tftp root
>>>> To Drew:
>>>> --------
>>>> To address the ps(1) pain, I consolidated the function down and
>>>> filed 11524 - Should look to using PSI (Python System
>>>> Information) for Python process management
>>>> I looked into Bill's bootadm work but I don't fit an "alternate
>>>> root" environment and I'd still need to provide a lot of parsing
>>>> anyways.
>>>> To Sundar:
>>>> ----------
>>>> I think our phone call Thursday cleared up your questions?
>>>> For those in the code walk-through:
>>>> -----------------------------------
>>>> I chose to append our findings of being able to have both a SPARC
>>>> and X86 client to a bug on create-client rather than address
>>>> finding all possible nooks for a client and spewing lots of not
>>>> found messages to a user (or having to catch the messages in
>>>> funky ways).
>>>> Jack:
>>>> -----
>>>> Per the agreement between Drew's coding style suggestions, those
>>>> of PEP8's hanging indents and Google's Python style guide I've
>>>> followed PEP8/Google's Style guide, however, I hope next Tuesday
>>>> we'll have time to come to a consensus on Python style there as
>>>> this should expand past this one push, of course. Thank you for
>>>> getting me to think about this so much!
>>>>
>>>> Thank you,
>>>> Clay
>>
>>
