Support some operating systems other than Linux

[chris-se]

There are efforts in Debian to support other kernels than Linux with the GNU userland, namely FreeBSD and GNU Hurd. This series of patches adds support for FreeBSD (both native FreeBSD and GNU/kFreeBSD) as well as Hurd to open-isns.

There are some minor differences when it comes to socket-related things in all of these operating systems, mostly related to credentials passing over AF_LOCAL sockets.

A variant of these patches is currently present in the packages in the Debian archive; I've cleaned those patches up and reworked them a bit - and I did functionality tests on Debian Linux, native FreeBSD, Debian GNU/kFreeBSD and Debian GNU/Hurd. The patches do not check for specific operating systems, but rather for the presence of certain constants, so this might also make open-isns work with other operating systems if they are similar to either Linux or FreeBSD in these aspects.

There's nothing that open-isns currently does that is really Linux-specific (in contrast to e.g. open-iscsi), so it would be really great if you could merge these patches. Thanks!

[gonzoleeman]

I am a bit uncomfortable with the fact that we will allow any length pathname on all platforms to support one platform that does not limit them. Your patch for Hurd calculates the amount to allocate by using "strlen" on the passed-in values. Perhaps I'm being too paranoid? Please convince me.

The FreeBSD patch looks fine. I assume you have tested both on Debian, i.e. the code still works correctly on Linux, correct? I have compile-tested them on SUSE Linux.

[chris-se]

I am a bit uncomfortable with the fact that we will allow any length pathname on all platforms to support one platform that does not limit them. Your patch for Hurd calculates the amount to allocate by using "strlen" on the passed-in values. Perhaps I'm being too paranoid? Please convince me.

Well, what happens with the values is that they are then passed to syscalls. (Such as stat, open, etc.) And if the name passed to a syscall is too long, the kernel will just return ENAMETOOLONG and the syscall fail, giving the user an error message. In contrast, if stuff is too long now, stuff gets truncated without any feedback to the user. So I think in any case this is an improvement.

However, in practice this will not really be a problem. Going through the occurrences of where I now concatenate paths:

https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-a721745503dd45f4033bc1db0ed0a1bbR98 https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-a721745503dd45f4033bc1db0ed0a1bbR110

Here only a numerical index (max. 32 bytes) is appended to the directory name (/var/lib/isns), so that should never be too long anyway.

https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-a721745503dd45f4033bc1db0ed0a1bbR374

The name of the file within a directory is concatenated with the name of the directory. If the file already exists, this shouldn't be a problem.

https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-a721745503dd45f4033bc1db0ed0a1bbR406 https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-a721745503dd45f4033bc1db0ed0a1bbR433

Only "DB" is appended to an existing directory.

https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-a996278f8a0cf3c415126c406e5b0755R1058

If the user specifies a client name that's unrealistically long, this might be a problem. But what's then going to happen is that the user will get an error message.

https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-d744841052adb24fcb127f446aabb5f0R143

If the admin configures a lock path that's ridiculously long, then it's their own fault.

https://github.com/open-iscsi/open-isns/pull/8/commits/15824eb06c78b8a003e985cd364387c3dda47ccd#diff-bcb36ac38577442f14d61ab62728fee1R503

This is immediately checked with access() if the file exists, so that's never going to be a problem.

So I don't think any of these occurrences are in any way problematic. If you still disagree, I'd be fine with adding some additional checks for the length before any of these paths is used if PATH_MAX is defined. (I would use the same code paths for allocating the strings on all platforms though, otherwise this will become an #ifdef mess.) Your call.

The FreeBSD patch looks fine. I assume you have tested both on Debian, i.e. the code still works correctly on Linux, correct?

Yes, I've functionality-tested the package on Debian with Linux, FreeBSD and Hurd kernels (GNU userland), and also on a current FreeBSD itself (BSD userland). In case you're wondering, what I've run is basically what I've also added as a CI test to the Debian package (isnsd --init is done at package installation on Debian, in case you're wondering why that isn't there):

https://anonscm.debian.org/cgit/pkg-iscsi/open-isns.git/tree/debian/tests/auth

Thanks for your quick response, and thanks for merging the OpenSSL patch. :-)

[gonzoleeman]

Okay, I'm convinced. Thank you for the detailed explanation.