Hi Sarah,
On 04/06/09 16:18, Sarah Jelinek wrote: > jan damborsky wrote: >> Hi Sarah, >> >> >> On 04/06/09 15:18, Sarah Jelinek wrote: >>> jan damborsky wrote: >>>> Hi Joe, >>>> >>>> >>>> On 04/06/09 13:57, Joseph J VLcek wrote: >>>>> Hi Jan, >>>>> >>>>> Thank you for making the changes. >>>>> >>>>> Sundar's question about invoking ls_init() made me realize one >>>>> more issue. >>>>> >>>>> >>>>> usr/src/cmd/auto-install/auto_install.c >>>>> ---------------------------------------- >>>>> >>>>> 1628 if (ls_init_attr != NULL) >>>>> >>>>> I believe this check, done on line 1628 for ls_init_attr != NULL, >>>>> should also be done before line: >>>>> >>>>> 1623 nvlist_free(ls_init_attr); >>>> >>>> You are right - good point. I have added that check. >>>> >>> Actually, the check for NULL isn't necessary. If you look at the >>> nvlist_free() code, it checks for NULL and does the right thing. >>> Either way is fine with me, I just wanted to point this out. >> >> This is a good point. I would prefer to remove those >> checks. But I am not sure we could rely on this behavior >> since man page for nvlist_free(3NVPAIR) doesn't mention >> this. Please let me know what you think. > Hi Jan, > > I looked at the arc case to see if they documented this behavior, they > don't specifically document that if it is a NULL value no operation > occurs. This case was filed in 2000 and this code has been in there > for a while. I think that we can rely on this behavior since it has > been for this long. It is my opinion it isn't likely to change. You > could file a man page bug so that this gets documented properly, much > like free(NULL) behavior is documented. Thank you very much for digging into this - based on your observations it seems that it is save to change the code. I will file bug for man page. > > Also, looking at your code, it doesn't seem to me that we would ever > have a case where the list could be non-NULL at any point you are > checking. Even if ls_init() failed it doesn't NULL and free the passed > in list. Actually, ls_init_attr is not NULL only if debug mode is enabled - otherwise it is initialized to NULL on line 1560. If anything changes in that area (which seems unlikely at this point), we would realize it soon :-) > > Bottom line.. I don't think we need to check for NULL before freeing > the list. I have changed the code and removed checks for NULL. Thanks a lot ! Jan
