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 [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.
