On 07/05/2011 12:08 PM, Jim Ramsay wrote:
> 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?
Ehhh, I do not really care either way. No one else seems to care, so do
what you are most comfortable with wrt your employer's requirements.
>
>> 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:
>
Ugghhh, I remember that now. I think it was added after the initial doc
a couple years ago maybe. Ok, you are right. Leave it as is if you
prefer it.
> --------
> 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.
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
