[caiman-discuss] code review for 1333

[Ethan Quach]

Jean,

Thanks for the review..

Jean McCormack wrote:
> General: Nice comments. It was refreshing to be puzzled about something, 
> look, and see it explained in the comments.
>
> be_rename.c
>
> line 70: I believe you now need to init zhp to null because line 145 
> does a goto done before 149 sets zhp for the first time.
>   

Yep.

> line 144 & 182: Should that be err=1 ?
>   

Yes it should.

> be_utils.c
> line 1245 & 1286  & 1305 (?): Don't you need to close(fd) ?
>   

The successful fdopen() at 1218 associates the tfile stream with fd.
The fclose(tfile) in the 'cleanup' should close that associated file 
descriptor.
There's an explicit call to close(fd) at 1221 because tfile failed to 
initialize
and is NULL there.

> line 1255: Is this still a todo?
>   

Unfortunately it still is for now. When we add in support for non-legacy
mounted roots. These TODO lines 1255 and 1330 will need adjustments.

> be_mount.c
> in _be_mount you're leaking tmp_altroot in most cases.
>   
> line 318: You copy altroot to tmp_altroot and then use tmp_altroot in 
> subsequent code and
> at the end, copy tmp_altroot to altroot if altroot was null. Why not 
> just use copy tmp_altroot
> to altroot around 310 and then you don't have to do this? 

Because if the function failed somewhere in the middle, I don't want
*altroot to have been set.

> It also might 
> make free'ing the calloc'd
> memory cleaner.
>   

If we're in the case that we've had to calloc, we need to return that
to the caller (see the function block Description), hence we actually
don't want to free the memory in this function for a successful return.

... and this has pointed out something else to me though. For failure cases,
327, 342, 349, we need to free(tmp_altroot) if it was allocated.

> be_get_legacy_fs: I think you need to zfs_close(zhp) before the various 
> return(1) 's or instead of return(1) do ret=1; goto cleanup;
>   

Yes. This needs to be done at 594 and 602.


thanks,
-ethan

>
> Jean
>
>
>
> Ethan Quach wrote:
>   
>> Can I get a review for this bugfix.
>>
>>
>> be_update_vfstab() now gets a list of file systems to
>> look for to update, rather than just looking for "/".
>> It also now restores the permissions and ownership of the
>> updated vfstab with those of that of the original file.
>>
>> Also, as a result of redoing be_update_vfstab(), I found
>> it would be better to change _be_mount() to generate
>> a temporary mountpoint if a mountpoint isn't provided,
>> instead of expecting a mountpoint to be passed in.
>> I think this will come in useful in the future for other
>> places where we'd need to internally mount a BE to do work.
>>
>>
>>
>> Defect:
>> ------
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=1333
>>
>> Webrev:
>> -------
>> http://cr.opensolaris.org/~equach/webrev.1333
>>
>>
>> thanks,
>> -ethan
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>   
>>     
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>