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.
/*
* 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 */
> 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.
> 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
*/
> 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."
> 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"
> 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).
> -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
>>>
>
>
