Mike Christie wrote:
>On 06/28/2011 03:26 PM, Jim Ramsay wrote:
>>
>> Copyright (c) 2011 Dell Inc.
>
>I have never seen that before. What is the copyright on or what is that
line
>for? I am more used to linux kernel development where people add their
>copyright to the source/header files they change if they have
contributed
>something significant to them like a new feature or like 5% of the code
or
>something.
The copyright is on the patch itself. The point is that while I haven't
changed any of the individual files enough to warrant adding Dell's
copyright line, I am also required to claim copyright on the code I did
actually write. I figured that having a copyright notice in the patch
(and also the resulting git history) but not the actual file would be a
reasonable compromise. Perhaps I could make this more clear by changing
the line to something like
"This patch copyright (c) 2011 Dell Inc."?
On the other hand, if you would consider these new features I'm adding
(multiple-sessions-per-iface and leading-login) as significant enough, I
could just add the full copyright line to the headers of iscsiadm.c and
session_mgmt.c since that's where most of my changes are taking place.
Which way would you prefer?
>Just some minor coding style nits below. Thanks.
I'm glad to fix these up and will re-submit them shortly. But before I
do, there is one area where I'd like some clarification:
>> + if (rec && rec->session.info) {
>> + num_found = 1;
>> + err = fn(rec, rec->session.info);
>> + } else {
>> + err = iscsi_sysfs_for_each_session(rec, &num_found, fn);
>> + }
>
>We follow the linux kernel coding style (mostly) where we do not add
{}s if
>they are not needed. So since there is only one line here we would not
add
>them.
I know you say "mostly", but this is actually one of those cases where
the Kernel coding style has an exception to its general rule:
--------
Do not unnecessarily use braces where a single statement will do.
if (condition)
action();
and
if (condition)
do_this();
else
do_that();
This does not apply if one branch of a conditional statement is a
single
statement. Use braces in both branches.
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
--------
If you really do prefer the alternative, omitting the braces around that
"otherwise();" in their example, I would be glad to comply. I just
thought I should double-check on this one since it's a special case.
Also, the issue you mention on Patch 3/4 (unexpected newlines and '=3D')
is troubling. I sent the entire series at once with 'git send-email'
with git 1.7.5.3 - No idea what would be going wrong there, unless some
SMTP server somewhere in the way is mucking with the data (some odd
transfer encoding or something?). If this happens again I can just push
my branches to some public repo and you can pull directly.
--
Jim Ramsay
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
