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.
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.
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.
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.
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.
line 90 - maybe "Make sure we have what's needed to run SMF" ?
libaiscf_instance.c
line 264, 316 - Closure should be closure so that it matches the actual
parameter.
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...)
-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
>>
