On Monday, June 29, 2015 at 1:38:04 PM UTC-7, Christian Iversen wrote:
Hello Open-iSCSI
(please CC as I'm not a regular on the list)
Please join the list if you are going to submit patches.
I've been working with iSCSI lately, and thought I could help with a few
patches.
Here's a description:
I am looking at your patches (I'm sure Mike will, as well), but my first
response is that your descriptions (and hence the names of your patches)
are not very descriptive.
For example, for your patch 8, remove dead code is too generic. How about
iscsiuio: remove unused ipv6 header.
-- 0001-Enable-MaxOutstandingR2T-negotiation-support-in-iscs.patch
This patch enables MaxOutstandingR2T negotiation for sessions.
Currently, max_r2t negotiation is in a weird, half-broken state. The
kernel seems to support max_r2t 1 just fine (at least it connects),
but iscsiadm is hard-coded to use 1, regardless of the setting that
the user sets in the config file, which is very confusing.
I spent more time than I'd like to admit on tracking this one down.. :)
There's even error handling in place for max_r2t 1, but this is never
triggered since the value 1 is hard-coded, and the config file is
ignored. This is the worst of both worlds.
-- 0002-Removed-a-number-of-unused-variables.patch
Quick cleanup, to fix a number of compiler warnings.
-- 0003-Removed-unused-variable-and-computation.patch
Same as 0002.
-- 0004-Added-missing-pointer-dereferencing-memset-would-onl.patch
-- 0005-Added-missing-pointer-dereferencing-memset-would-onl.patch
In the duplicate(!) md5 function MD5Final(), there is a final
memset(ctx, 0, sizeof(ctx));
to clear the context of sensitive data after MD5 calculation - which is
fine.
Unfortunately, they both refer to sizeof(ctx), which is a pointer,
effectively nullifying (no pun intended) the effect of memset(). This is
changed to sizeof(*ctx).
-- 0006-Removed-unused-variable-one.patch
-- 0007-Removed-unused-variable-pdu_text.patch
-- 0008-Removed-dead-code.patch
Should be self-explanatory.
Not really.
These are all more or less just compile-tested. Any comments?
Yes, should be run-tested as well if you are going to submit this many
changes, IMHO.
Also, I'd suggest grouping similar changes together. Why create a patch for
each variable removed? If you are cleaning up unused variables, I would
group those patches as one. But that might just be me. :) And the two
memset() fixes should be one patch.
Lastly, you cannot patch open-isns in open-iscsi any longer. You need to
submit those changes to https://github.com/gonzoleeman/open-isns
--
De bedste hilsner / Best Regards,
Christian Iversen
System Engineer
Meebox ApS
Store Kongensgade 40H,
1264 København K
T: +45 3841 3841
--
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 open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.
