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 95: kililng --> killing 100: If the instance is not service, it is better to print an error indicating that the input is not a service 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. 127: "run on your DHCP server" --> run the following command on your DHCP server 294: txt_record --> image_path 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. 347: We don't use the term microroot any more. Please change it to 'boot_archive' 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. installadm.c: 181: strcmp returns an int. Check for int rather than boolean 944: Does delete-service.py returns 0 on success or error code on failure? 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. 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? 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 libaiscf_service.c: 146, 200: Same comment as 288 of libaiscf_backend.c 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. Thanks, Sundar
