Susan Sohn wrote:
> Hi Joe,
>
> On 03/14/09 03:53, Joseph J. VLcek wrote:
>> Sue Sohn wrote:
>>> Please review the changes for:
>>>
>>> 7388 create-service not reusing port number on a disabled service
>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7388
>>>
>>> and
>>>
>>> 7275 /etc/netboot/wanboot.conf has wrong permissions
>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7275
>>>
>>> which are posted at:
>>>
>>> http://cr.opensolaris.org/~sohn/7388_7275
>>>
>>> Thanks,
>>> Sue
>>> _______________________________________________
>>> caiman-discuss mailing list
>>> caiman-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>> usr/src/cmd/installadm/installadm_util.c
>> ----------------------------------------------------------------
>>
>> Suggestion:
>>
>> 386 if (access(path, F_OK) != 0) {
>> 387 return (B_FALSE);
>> 388 }
>>
>>
>> I would rather see the call to access be done in function
>> read_service_data_file()
>
> Any particular reason? I would actually prefer to leave it where it is,
> since why try to read the file with read_service_data_file unless you
> know it exists?
I think this would make read_service_data_file() more able to stand on
it's own. Early in read_service_data_file() it should validate it's
arguments. Even if this includes validating if "path" is accessible.
If the issue is that you simply don't want the error issued at line
390/391 then you could check access(path) only if remove_service_data()
fails and add a comment indicating why the error message is not displayed
e.g.:
390 if (read_service_data_file(path, data) != B_TRUE) {
391 (void) fprintf(stderr,
MSG_READ_SERVICE_DATA_FILE_FAIL,
392 path);
393 return (B_FALSE);
394 }
395 return (B_TRUE);
396 }
397
To:
if (read_service_data_file(path, data) != B_TRUE) {
if (access(path, F_OK) == 0) {
(void) fprintf(stderr, MSG_READ_SERVICE_DATA_FILE_FAIL,
path);
return (B_FALSE);
}
}
return (B_TRUE);
Or update read_service_data_file() so it returns an enum indicating why
it failed.
Joe
>
>> usr/src/cmd/installadm/installadm.c
>> ---------------------------------------------------------
>> Looks OK.
>> I did see Sanjay's comment about do_create_service() being large and
>> deeply nested.
>> I agree addressing would be good and would have made reviewing the
>> code easier.
>
> I agree, however as I said in my response to Sanjay, I would like to
> open a new bug to track that work.
>
> thanks,
> Sue
