Hi Jean, Thanks for the review. Comments inline ...
thanks, -ethan Jean McCormack wrote: > be_mount.c: > line 348: I'm confused as to why you do the be_get_uuid call. uu is > never used after it and > it's not clear there's any side effect. So why do so? If there's > a reason a comment would help. > Its required that the BE has a uuid for zones to be supported. The process of getting the uuid validates it has one and is in the right form. I'll add a comment to note this. > line 351: Do you need the if(gen_tmp_altroot) free(tmp_altroot); > code here? > yes, thanks. > line 477: ditto > There's no gen_tmp_altroot in _be_unmount(). > line 545: Aren't we supposed to use strncmp for security reasons? > > line 598: ditto > It may be a security issue if one of the strings is a not a null terminated string, but we could still run into that with strncmp can't we? > line 645: Shouldn't these really be be_errno_t? > > line 858 & 861: shouldn't this really be be_errno_t? (Actually this > is going to be a generic comment > since I see it many places) > I would rather leave them all as ints. The definition of the be_errno_t is the enumeration of possible error codes, and the functions return one of them. > line 679: Nit (there always has to be at least one nit). Remove the > get, it's already on the line above. > okay. > line 828- 841: What happens if the strlcpy actually truncates dir? > I suppose that if a truncation happened at exactly a point that left a valid directory name, the code could lookup up the wrong dataset. We'll validate the length of dir upon entry. Thanks. > line 851: Is this really none? Maybe something here or in the returns > about tmp_mp. > Yes. I will update this. > line 867: should this be strncpy? > We explicitly allocated *tmp_mp at line 863 to be the exact length required for BE_TMP_MNTPNT, so I think a strcpy here should be sufficient. > line 940 & 942: should this be strncmp? > Same comment as for 545,598 > line 1447: Huh? > Should be " at the altroot" > > line 1895: do we need to free altroot? > If the call to make_tmp_mountpoint() failed, we don't have an altroot. > line 2175: Maybe file a bug on this? > We will once we implement shared file systems for zones. thanks, -ethan
