Iscsi+smbfs #621
Iscsi+smbfs #621
Conversation
|
Ok, managed to figure out why I got the Assertion. Fixed locally. Trying to find a solution to point two before I push to github. |
|
Please take a look at the inline comments. I'm happy to make a another pass when you pushed a reworked version. I'd also really like to see the DEBUG code dropped because at this point we shouldn't need it and it really hurts readability. Thanks for working on this! |
|
Fixed all this in a new push (commit 54711e52810f55f5b8152b3a3cc253c5d5386601). EXEPT the TODO function! Should I rebase my changes and then update my pull request? |
|
Yes, please. Go ahead and rebase your changes squashing the fixes in and force repush the branch. |
|
Ok, rebased after five or six tries! That command is HARD!! :). Many of your previous comments on the SMB patch could probably be applied to the iSCSI patch as well, but I'll wait for your comments to be sure :). |
|
|
||
| /* ====== */ | ||
| /* Part 3 */ | ||
| argv[0] = (char*)"/sbin/zfs_share_iscsi.sh"; |
There was a problem hiding this comment.
This command is never actually called.
|
@FransUrbo I have a renewed interest in landing SMB support for this project soon and would like to help push it forward. Would you mind if I look through the patch and clean up some of it's rough edges? |
|
Most of what's mentioned here, have already been fixed. It might not show, because I have to do a force push to Github which overwrites the code discussed. And Brian and Richard is already reviewing the code, and I'm fixing it as soon as they have given me opinions. OR, figure out why it doesn't share automatically when you 'set shareiscsi=on' like the smb/nfs do. |
|
Can you remove commit, 32312b9, from the pull request. It looks like that is not intended to land in the first place, so it only adds confusion when reviewing the patches. Send me a email/message if you are unfamiliar with git and are unsure how to do this, I'll try to help you out. |
|
Ok, a new rebase and a forced push have been done, fixing issues mentioned. |
|
Thanks for the refreshed patch. I'm going to actually try and use it this weekend and see what sorts of issues I run in too. |
@FransUrbo What sort of behavior did you observe? Does it only occur in the presence of these patches? If not, is there an open bug on it? |
|
I can't remember if we ever figured out exactly why/when it happens. But it might have something to do with 32/64 bit 'conversion something' :). It do happen without these patches and the behaviour is that zvol_id can't figure out the device path. It only gives garbage or nothing at all. Check the list, two/three months ago. |
|
The zvol issue is probably unrelated. There were some reports of stack smashing occurring in issue #569. I made a start to cleanup this little helper utility but didn't quite get it done and tested. WIP here: |
|
@FransUrbo, In that case, it might not be appropriate to address that issue as part of this commit. Note that I have a downstream motivation for saying this. Gentoo's ZFS ebuilds filter CFLAGS against a white-list that is global to the distribution. Users that know enough to provide useful bug reports are free to disable it. If the CFLAGS change is committed, I will be expected to revert it downstream. |
|
@FransUrbo I am encountering the following error: vserver ~ # zfs share rpool/KVM/zfs-test Looking at the code, getdomainname is returning ENOENT. getdomainname is a GNU-extension implemented by glibc [1]. Not all Linux distributions use glibc as their system library and while uclibc also implements this, that cannot be guarenteed for other libc implementations [2]. It might not be a good idea to rely on such extensions. As far as I can tell, what it actually provides is poorly defined [3], which is probably why it is broken on my system. Perhaps we could use gethostname here instead. That should be the equivalent of 1: http://linux.die.net/man/2/getdomainname |
| /* PART 1 */ | ||
| snprintf(params_name, sizeof (params_name), "Name=%s", sharename); | ||
|
|
||
| /* int: between -2,147,483,648 and 2,147,483,647 => 10 chars + NUL */ |
There was a problem hiding this comment.
Why is tid_s 16 characters in size then?
| @@ -908,8 +908,6 @@ rpm-modules: srpm-modules | |||
| rpm-utils: srpm-utils | |||
| $(MAKE) $(AM_MAKEFLAGS) pkg="${PACKAGE}" rpm-common | |||
|
|
|||
| rpm-modules: srpm-modules | |||
There was a problem hiding this comment.
What is this change for? Can it be part of a separate pull request?
|
@FransUrbo The issue with strlen inside of loops is that it is (usually) loop invariant, but the compiler is not capable of the analysis necessary to determine that, so it will be executed each iteration, wasting CPU resources. In cases when it is not loop variant, you can usually use a counter to keep track of what its output would be. I suggest that you rework code using it to only call it once at the beginning of a loop. Store the result in a separate variable and work with that instead. |
|
@FransUrbo Isn't a |
|
Actually, that's not safe either. Assuming you just want truncation, do this: |
|
Yes, indeed my example was not entirely complete. You need the assignment of |
|
But size of on a 'char*'... ? |
|
@FransUrbo After you rework the code copying strings, it might be worthwhile to open a new pull request. There is a great deal of clutter in this one and it becoming difficult to follow. |
|
Where is the clutter, and what´s wrong with 'Misc personal changes'? Is there a way to remove only that part from the pull request, but not for my commits? |
|
New push after fixing: |
|
Thank you for your commits. When Brian merges it, i would like to try. Did you observe, that the names of the zvol devices ( /dev/zd*) changes after reboot, when |
|
If this is true, it is not because of my patch and I have not noticed this. But even if this is so (that they change name), it does not matter. Not from the SMB/iSCSI patch point of view. If you find this a problem anyway, please file a bug report about this or take it up on the mailinglist. And you can still test this/these patch(es) by checking out my branch. |
|
Ok, |
Sixth official patch attempt. Previous ones was/is lost
in my inability with GIT.
* Fix issues after review by behlendorf.
= Move comments etc from the README to the code and to the manpage.
= Adhere to the 80 char line limit.
= Cast constants when setting argv[x].
= More defines, concentrated at the top.
= Move smb_retrieve_shares() to the top, to avoid forward declaration.
= Make private functions static.
= Remove all debugging.
= Whitespace fixes.
= Return SA_OK instead of 0 where needed.
= Remove some fprintf(stderr, "ERROR: ...") strings. Not needed really,
they where mostly for debugging any way.
= Bump release.
= Add missing locale.h include.
= Add file_is_executable().
= Remove file_is_executable() per request from Brian.
* Some minor SMBFS fixes, comments from Richard Laager:
= Remove redundant variable = NULL check.
= Rewrite static 'net' command options define's
= Remove commented out/redundant line parsing code.
= Add Samba host options to net command in smb_disable_share_one()
as well.
* Fixes after second review by behlendorf.
= Putting back things that wasn't mine that I removed or
changed..
= Remove the file_is_executable() declaration that came
back some how.
= Reword, rewrite and indent information about how to
setup Samba.
= Implement smb_validate_shareopts().
* Fixes after third review by Prakash-Surya.
= Removed missplaced space.
Sixth official patch attempt. Previous ones was/is lost
in my inability with GIT.
* Script to run through some tests to test share/unsharing...
* Debugging output added.
* Retabbing
* Fix faulty nfs|smb check (setting ZFS_TYPE_FILESYSTEM). Now possible to
use 'share -a [nfs|smb|iscsi] as command line suggests. Not documented
in man page though...
+ In unshare_unmount(), 'if(do_all)', 'continue' instead of 'break' after
checking 'sharenfs=off' (so that we can continue checking for shareiscsi
and sharesmb).
* Process iscsi shares early in update_zfs_shares_cb().
* Fix problem with assertion fail when running 'zfs unshare -a'.
* Call process_share() early of update_zfs_shares_cb() if iSCSI.
* Add shell script (test_zfs.sh) to test some functionality of SMB/iSCSI
patch(es).
+ Remove file_is_executable() from iSCSI per request from Brian.
Fix some stuff that I know Brian will nag about :)
* 80 char line
* Move content of README to code + man page.
* Remove all debugging.
* Return SA_OK instead of 0 where needed.
* Cast to char* when setting argv with constant string(s).
* C9x compliance
* Make private functions static.
Some minor iSCSI fixes, comments from Richard Laager:
* Remove redundant variable = NULL check.
* Rename the custom iSCSI share script (remove .sh suffix).
+ Reword manual
Some more iSCSI fixes.
* Since iscsi_retrieve_targets() is now static, move it
up to the top of iscsi.c and remove from iscsi.h.
* In update_zfs_shares_cb(), do the iSCSI parts later
and skip the if type != FS check etc.
* Implement iscsi_validate_shareopts().
* Distinguish between FS and VOL when updating
share information.
+ Support adding a VOL to list.
* Uncomment some stuff in process_share() that is/was supposed
to possibly fix the 'shareicsi=on' auto sharing but didn't.
Only made it to not compile...
* Update copyright note and year(s).
Fixes after second review by behlendorf and comments by Richard Yao.
* Check for getdomainname() - set HAVE_GETDOMAINNAME=1 if so - in
./configure.
+ Only try to get domain name with getdomainname() if
we have it (HAVE_GETDOMAINNAME). If we don't, use the
file /etc/domainname (configurable with DOMAINNAME_FILE).
* s/if\(/if \(/.
* Init iscsi_volumes_fp in iscsi_retrieve_targets() to NULL.
* Make sure tid_s is only 11 chars long.
* Remove one level of indentation from test binary.
* Don't malloc temp - memleak in test binary.
* Retabbing.
* s/share/tank/g in comments etc.
More fixes after review by Richard Yao, Richard Laager and Prakash Surya:
* Spelling errors fixed.
* ietadm is in man section '8', not '8M' (to much cut-and-past :).
* Remove (rewrite) some redundant strlen() usage.
* Replace a non-working sizeof with strlen.
Other changes and fixes:
* Update Release to indicate date and branch.
* Remove trailing newline/null in domainname (was cutting a char to
much).
* Put the extra share script (/sbin/zfs_share_iscsi) as a define.
* Optional /etc/iscsi_target_id which contain the TID - this instead
of autogenerating one (which changes every month!).
* Rewrite domain name information for iSCSI.
* Document new optional /etc/iscsi_target_id and it's use.
* Update list [iscsi] targets using iscsitadm with something that
works in Linux.
|
@FransUrbo First off thanks for keeping these patches refreshed. I know they've been pending for a long time, but I think we can partially move on this. The samba patches are largely self contained and quite safe. So I'm willing to pull those changes in to master for the next release if your willing to maintain that code. I just don't have the time to spend any cycles on fixing samba integration issues. The iscsi changes are more disruptive and frankly the right solution here is a lot less clear. But at least this gets you down to a single patch. |
|
On Nov 15, 2012, at 10:11 PM, Brian Behlendorf wrote:
|
|
@FransUrbo Sounds good. Just two requests before I pull in the samba changes.
|
|
I might have jumped the gun a little there. When running through my checks again, I noticed that although a unshare worked, the zfs command claimed that it was unsuccessful because it couldn't find the 'unshare' command. Fixed that, but it required some changes to lib/libzfs/libzfs_mount.c that you might not like (disruptive, same as the iscsi patch - not as big, but still). I'll do a new push shortly for you to review. In the meantime, how about this wording for the man page: Also, I don't currently support any optional parameters to sharesmb (because I can't figure out how sharenfs does it! :). If anyone have any interest in lending a help, I'll be grateful. |
|
@FransUrbo I'll review the new changes you needed to make and we should be able to get them in a form which is acceptable. Plus if we accidentally break something now I know who to assign the open issues too! At least this will help lay the ground work and make it easier for other to contribute. The updated man page looks pretty good. The real test will be if I can successfully setup a ZoL system to use Samba using those directions. |
|
Ok, I've tried three times now, but I still get 11 commits! I have done a rebase against master and in the git logs all looks ok... Any ideas? |
|
By the way, could you close the 'Iscsi+smbfs' pull req? I'll do a new one with only the iscsi part so we have two separate ones... |
|
Closed |
Signed-off-by: Paul Dagnelie <pcd@delphix.com>
Both iSCSI and SMB support into one pull request. Solves #492 and #493.
Current problems:
zfs: ../../cmd/zfs/zfs_main.c:5915: Assertion `zfs_prop_get(zhp, ZFS_PROP_SHAREISCSI, nfsiscsi_mnt_prop, sizeof (nfsiscsi_mnt_prop), NULL, NULL, 0, B_FALSE) == 0' failed.