share/unshare SMB support

[FransUrbo]

zfs create tank/share
zfs set sharesmb=on tank/share
zfs share tank/share
zfs unshare tank/share
zfs share -a
zfs unshare -a

[behlendorf]

Thanks! When a get a little time I'll take a look at this. I'm unfortunately a little swamped this week and next.

[dajhorn]

@FransUrbo: I will do a code review for you. I looked at the SMB enhancement.

1. Coding Standards

Everybody trips over this when they make their first contribution.

• The debugging statements must be removed, or indented and wrapped, according to the Sun coding standard.
• In smb_retrieve_shares, consider using strtok, which would be more like the coding style elsewhere in ZoL.

2. Character Classes

The Microsoft SMB specification defines the share name character class in an exclusive fashion, as not:

 "/\[]:|<>+;,?*=

The regex set for a ZFS name is:

 [[:alnum:]_.: -]

This means that the smb_enable_share_one function should also substitute the :, -, and space characters in addition to the / character. For example, libshare must handle this kind of configuration:

 # zfs list -r testing
 NAME                USED  AVAIL  REFER  MOUNTPOINT
 testing/foo bar     30K   344G    30K  /testing/foo bar
 testing/foo-bar     30K   344G    30K  /testing/foo-bar
 testing/foo:bar     30K   344G    30K  /testing/foo:bar

3. Intellectual Property

In smb.h, you are giving Oracle undeserved credit.

In smb.c, Jerome Bettis has not licensed anything to the ZoL project -- not explicitly nor implicitly -- so the strrep function should be removed because it a contamination risk.

Regardless, the code is inefficient because the character translation can happen in one pass without calling strchr severally, like this:

 char *pos = str;
 if( pos == NULL ) {
    return;
 }
 while( *pos != '\0' ) {
    switch( *pos ) {
       case '/':
       case '-':
       case ':':
       case ' ':
          *pos = '_';
    }
    ++pos;
 }

4. External Dependencies

If the /usr/bin/net utility is not installed, or if /sbin/zfs is invoked as a non-root user, then it would be nice to get a warning and a non-fatal return.

The libshare/nfs.c implementation already has bug reports for this behavior.

[FransUrbo]

PS. About the root check - didn't fix that part. In theory, since I'm using 'usershare' instead of 'share', ANY user could create a SMB share. Provided the correct samba config is setup and user have write access to /dev/zfs.

I've removed the hardcoded '-U root' from the commandline to allow non-root users to create SMB shares though and documented how to do this as non-root user.

[dajhorn]

@FransUrbo: I tried your combined branch and noticed that there are instances of Author: root <[email protected]> instead of your registered Github account name.

Consider updating these commits so that Github can automatically track and credit your contributions:

• 1d3ea49c0479e4d51102d4357fd0d33b93577254
• 5af70b7d0cc19124662ff880076e642ae77188b9
• 17e186f85eb28412ca8a7ba39856ff848ae4dcfd
• 38cf913b97d8f820f7e51351f2eeca6cba97290c
• 6ecadafe60104f47a83da5679fb69cd43f46339a
• 013d69927da7f5d050ba2f1e0a7a71ec8a289d5e
• a23f6db493a17d37c5d9d35b40daffdb73660f91
• 8217ed0
• 7d9d80d
• d686384
• b5493c8
• cb4f9ea
• b54231d

[FransUrbo]

On Dec 30, 2011, at 5:34 PM, Darik Horn wrote:

@FransUrbo: I tried your combined branch and noticed that there are instances of Author: root <[email protected]> instead of your registered Github account name.

Consider updating these commits so that Github can automatically track and credit your contributions:

• 1d3ea49c0479e4d51102d4357fd0d33b93577254
• 5af70b7d0cc19124662ff880076e642ae77188b9
• 17e186f85eb28412ca8a7ba39856ff848ae4dcfd
• 38cf913b97d8f820f7e51351f2eeca6cba97290c
• 6ecadafe60104f47a83da5679fb69cd43f46339a
• 013d69927da7f5d050ba2f1e0a7a71ec8a289d5e
• a23f6db493a17d37c5d9d35b40daffdb73660f91
• 8217ed0
• 7d9d80d
• d686384
• b5493c8
• cb4f9ea
• b54231d

How do I do that? I'm not a friend of git yet... :)

System administrators motto:
You're either invisible or in trouble.

• Unknown

[dajhorn]

@FransUrbo: This page has instructions for changing the commit author:

http://stackoverflow.com/questions/3042437/change-commit-author-at-one-specific-commit

[rlaager]

This line (and the one below) is unclear. How about if (file_is_executable(NET_CMD_PATH) != SA_OK) instead? Or, better, change the file_is_executable() function to return a boolean in the opposite sense (TRUE if executable, FALSE if not).

[behlendorf]

Closing pull request, a new one with be opened with the latest version of the patch.