Hi Sundar,
My apologies for not actually sending this, I thought I'd sent it
Friday.
Thank you,
Clay
On Fri, 11 Sep 2009, sundar Yamunachari wrote:
> Clay Baenziger wrote:
>> Hello all,
>> I've got a webrev for some mighty delete-service changes. The bugs I'm
>> hoping to address are:
>> 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
>> 10740 - Need way to interact with SMF from Python for installadm
>> components in Python
>>
>> Further, this webrev includes the already reviewed (but as yet unpushed and
>> related libpyscf code). The files to ignore (unless you're interested) are
>> anything under usr/src/lib/*
>>
>> Otherwise, if I could get initial comments as soon as possible, however,
>> Eric has extended the expected timeline for these changes to be pushed to
>> next week opposed to the initial hope of Friday.
>>
>> Webrev:
>> http://cr.opensolaris.org/~clayb/delete_service/
>>
>> Thank you,
>> Clay
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
> Clay,
>
> delete-service.py:
>
> 77: Comments same as 49 and it should be changed to client
Yup
> 95: kililng --> killing
Yup
> 100: If the instance is not service, it is better to print an error
> indicating that the input is not a service
No an error, but has been changed around to be more obvious as such
> 118-120: This is not entirely true. The user can provide a boot file as an
> option to create-service. The DHCP macro name in this case follows the case
> of service name. For example if the service is "my_service" and boot file is
> "my_file", the dhcp_macro will be dhcp_macro_my_file.
Yup
> 127: "run on your DHCP server" --> run the following command on your DHCP
> server
Yup
> 294: txt_record --> image_path
Yup
> 281: The function is called findImagePath but why is it printing "Not
> removing image path; %s is used by " in line 317. Is this function used only
> by delete-service? If more than one service can use the same image, then this
> function should return the image path. If one image can be used by only one
> service, then it should print the proper error message and the create-service
> should not allow such a scenario.
This is a nested function so it has no scope outside the remove_files()
function so it's not possible to be generally used. (I don't see a need
for it generally either since getting the imagePath is trivial in Python
now (smfInstance[<service name>]['image_path'])). However, it is possible
that two services will have the same image path (and reasonable too as
images can be rather large). Would you have another suggestion for this
function's name?
> 347: We don't use the term microroot any more. Please change it to
> 'boot_archive'
Yup
> 392: In the function removeTFTPBootFiles(), it is assumed that /tftpboot is
> the directory for TFTP. This directory is configurable. Please take a look
> at /etc/inetd.conf and check the tftp entry. It will be something like this:
>
> tftp dgram udp6 wait root /usr/sbin/in.tftpd in.tftpd -s
> /tftpboot
>
> For booting, we setup TFTP with /tftpboot as the root directory. Since it is
> configurable, it should not be assumed as /tftpboot.
Filed bugs:
11488 create-service/create-client: need to check inetd.conf for tftpboot
root
11486 delete-service/delete-client: should check inetd.conf for tftp root
I've addressed 11486 here.
> installadm.c:
>
> 181: strcmp returns an int. Check for int rather than boolean
Yup
> 944: Does delete-service.py returns 0 on success or error code on failure?
It currently returns a 0 on anything where there wasn't a serious error.
So if files are missing but it still does most of a remove it returns 0
and prints warnings to stderr. If it fully fails (i.e. not root, bad
options or SMF service not found) then it returns a 1. There are
provisions to make the return code more informative in the future (i.e.
success, human action suggested, failure) but installadm didn't look to be
doing more than checking for a 1 or 0.
> ai_utils.c:
>
> 168: Before retuning, scf_property_destroy() needs to be called. Check on
> lines 179, 193, and 199 whether the property to be destroyed.
The property is destroyed immediately after creation. I have put a comment
in explaining why and that it is, however.
> libaiscf_backend.c:
>
> 67: free the handle before returning. Do you need to free the handle in any
> of the failure returns in lines 92, 97 and 102?
This is called out of ai_scf_fini.
> 288: I can understand that you want to declare the variable after finding the
> len but declaration in the middle of code doesn't look right. Use malloc or
> calloc here instead or put the declaration and rest of the code inside a
> block.
> 399, 446 : same comment as 288
I would much prefer to use stack space rather than dynamic memory as then
the compiler can handle the clean-up and it's less likely to result in
accidental memory issues.
> libaiscf_service.c:
>
> 146, 200: Same comment as 288 of libaiscf_backend.c
Again, I understand this looks strange since for two decades out of C's
three, it was not legal C (this is a C99 construct) but here we just need
a temporary string and calling attention to it elsewhere is not very clean
or good looking (and it can't be initialized until after len is found).
> General comment: There is a installadm_common.py and installadm-common.sh in
> the source. What is the relation between them? If they are same, we need to
> remove the old one (installadm-common.sh). If they are different, please
> change the name of the python module since it will create confusing.
Yes, right now installadm-common.sh is used by the shell-script framework.
When those go away installadm-common.sh can go away. I'd like to group
these commands in an explicitly installadm grouping until we have a good
idea how to make our Python more reusable across all of the install code
and as it is the common framework for installadm Python code it seems
fitting to follow the same naming scheme?
> Thanks,
> Sundar
>
>
