Message forwarded from Christian Seiler:
On 10/30/2015 10:47 AM, Peter Hatina wrote:
in several projects, we identified the need to have a client-side library
for the
iSCSI communication.
Just out of curiosity: could you provide one or two examples?
This library has been living in Fedora for a while;
but
we would like to change this situation and upstream several patches from
Fedora.
If this is upstreamed, I think there should be a commitment about ABI
compatibility: that ABI breaks are very rare, and if they happen, the
SONAME of the library is consistently updated, so that no third-party
software breaks there. (Also, we should make sure that the built
libraries across distributions actually share the same ABI.)
Looking at the code, it seems to me that there are already a lot of
things in there that make a ton of sense and will ensure that it won't
be a burden to keep ABI compatibility (i.e.: I like):
- central library context + functions to create / destroy that
- proper namespacing of functions with libiscsi_
- a lot of getter/setter functions for different things
What I think could be improved:
- trivial: add $(LDFLAGS) to the linker rules in the Makefile. In
Debian we compile packages with hardening options by default (e.g.
-Wl,-z,relro), and we pass these to the build system. I sent a
couple of patches upstream earlier (that have now been committed)
that make sure that passing env variables is possible for the rest
of open-iscsi. Also, you should filter -fPIE and -fpie from the
flags building libraries, because they conflict with -fPIC, i.e.
use something like:
CFLAGS_LIB = $(filter-out -fPIE -fpie -pie,$(CFLAGS))
LDFLAGS_LIB = $(filter-out -fPIE -fpie -pie,$(LDFLAGS))
and use those flags in the rules. (If you want to overwrite the
original variables instead of creating new ones with a suffix,
make sure you use override.)
- enum libiscsi_auth_t: I'd prefer if explicit values were specified
in the enum, then it'd be harder to accidentally break binary
compatibility
- libiscsi_session_timeout: mention that all values are in seconds,
document semantics of special values (if any)
- the big one: fixed sized arrays in data structures: I really
dislike this, just because this
* the current use of just doing strlcpy() hides potential
truncation errors
* if the maximum length is changed at some point, this
immediately breaks ABI
The advantage of these data structures is that they simplify
memory management inside the library a bit, because without
fixed lengths, there's need for explicit destructor functions.
This also applies to simple values in some getters. So,
instead of doing something like
PUBLIC int libiscsi_node_get_parameter(
struct libiscsi_context *context,
const struct libiscsi_node *node,
const char *parameter,
char *value);
it would be better to simply have
PUBLIC int libiscsi_node_get_parameter(
struct libiscsi_context *context,
const struct libiscsi_node *node,
const char *parameter,
char **value);
where value is allocated inside the function and only if no error
occurred the pointer will be changed by the function. (Also,
tasking the user with making sure that the buffer they pass to
the function is large enough is a recipe for potential buffer
overflow exploits in client software.)
- next big one: some of the data structures. I think that the
following data structures should not be user-visible but rather have
initializers/destructors + getters/setters to manipulate them:
libiscsi_node
libiscsi_session_info
libiscsi_network_config
The reasoning as to why I think they shouldn't be user-visible is
the following: if at some point the structures are to be modified
(even if it's just adding a simple field), having them user-visible
will mean that any change will be ABI-incompatible. But I could
easily imagine that any of the mentioned structures might change in
the future due to improvements (e.g. information about whether the
connection is done in software or whether it's offloaded could later
be added to the node data structure).
libiscsi_session_timeout is fine as long as you are sure that no
other timeout will come - but OTOH, since it's only used in the
session_info structure, having a couple of getters for the different
timeouts might be a better idea.
The auth_info stuff I would keep as-is (with the only change being
that the CHAP version has char *username instead of
char username[MAXLEN] - see above). The semantics there should be as
follows: when the program passes information _into_ the library, the
library should only assume the structure passed is big enough for
the auth type specified, and for all information passed _from_ the
library libiscsi has to allocate the storage itself, to make sure
that the structure is always large enough.
- libiscsi is not entirely thread-safe (see the comments you wrote in
the code) and libiscsi can definitely not be used with multiple
contexts at the same time (due to the limitations of the underlying
code in open-iscsi it reuses).
This is not something that should prevent the addition of libiscsi,
but it should be clearly documented that that is the case - it
doesn't affect the ABI whatsoever, so it is perfectly reasonable to
say that initially it's not safe to use it more than once at the
same time in a program and then in a long-term effort try to improve
that situation by making more of the underlying code (that was
written with standalone binaries in mind) context-aware.
Nitpicks:
- do you really want to guard __attribute__((visibility("default")))
by __GNUC__ >= 4? GCC 4 was released a decade ago.
- you should document better that with "client library" you mean a
library that communicates with iscsid on Linux systems (and may
require some privileges), not an actual initiator implementation.
With that in mind: is libiscsi not a bit generic a name for
something that specific? A short search lead me to
<https://github.com/sahlberg/libiscsi>
<https://github.com/sahlberg/libiscsi>, so there's apparently
already at least one libiscsi out there that actually implements
the iSCSI protocol directly. Maybe use libopen-iscsi? Or
libiscsid-client?
- installing a pkg-config file would be really nice, especially if you
have sofware that uses this.
- it should be well-documented that the library should always match
the the installed iscsid version, because it internally uses the
communication protocol via the socket and that might be bound to
change between releases. This also means that there should be a big
fat warning against building the library statically and linking
against it, because that wouldn't be upgrade-safe.
I would be very pleased, if some of you, upstream developers, look at my
github
repository [1] with several changes regarding open-iscsi.
As I said, not an upstream developer, but hopefully you'll find my
feedback useful regardless.
There are some changes in that repository not related to libiscsi and I
would suggest treating them separately. (If they are intended for
upstream at all.)
Regards,
Christian
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
