Skip to content

Conversation

@nupuruttarwar
Copy link
Contributor

This patch is initial commit to introduce linux
networking support in krnlmon.

Title: Add linux networking support
Signed-off-by: nupuruttarwar nupur.uttarwar@intel.com

@nupuruttarwar nupuruttarwar marked this pull request as ready for review August 9, 2022 22:37
limitations under the License.
*/

* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should carry the Barefoot copyright as well as the Intel copyright.
It is a derivative of an existing work.

Copy link
Contributor

@ffoulkes ffoulkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Most of my comments are meant to be informative. The only point I consider important is that we must preserve copyright notices when we adapt existing intellectual property.

I have no objection if you want to wait until the next iteration to fix the copyright notices, especially if this you are doing little more than carrying over existing code from P4-OVS in this iteration.

Incidentally, if any of the new files were adapted from existing code, you should add their copyright notices back in.

See the License for the specific language governing permissions and
limitations under the License.
*/
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should carry the Barefoot copyright as well as the Intel copyright. It is a derivative of an existing work.

See the License for the specific language governing permissions and
limitations under the License.
*/
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should carry the Barefoot copyright as well as the Intel copyright. It is a derivative of an existing work.

* See the License for the specific language governing permissions and
* limitations under the License.
*/
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should carry the Barefoot copyright as well as the Intel copyright. It is a derivative of an existing work.

limitations under the License.
*/

* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both copyright notices

See the License for the specific language governing permissions and
limitations under the License.
*/
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both copyright notices

@@ -1,33 +1,29 @@
/*
Copyright 2013-present Barefoot Networks, Inc.
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both copyright notices

@@ -1,46 +1,39 @@
/*
* Copyright 2013-present Barefoot Networks, Inc.
*
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both copyright notices

@@ -1,11 +1,11 @@
/*
* Copyright 2013-present Barefoot Networks, Inc.
* Copyright (c) 2022 Intel Corporation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both copyright notices

@ffoulkes
Copy link
Contributor

ffoulkes commented Aug 9, 2022

On the next commit, could you please correct the title in the top-level README.md file to the name of the repository on ipdk-io? I accidentally carried over the repository name from intel-sandbox. Thanks.

@nupuruttarwar
Copy link
Contributor Author

On the next commit, could you please correct the title in the top-level README.md file to the name of the repository on ipdk-io? I accidentally carried over the repository name from intel-sandbox. Thanks.

Sure, will do

@nupuruttarwar
Copy link
Contributor Author

LGTM.

Most of my comments are meant to be informative. The only point I consider important is that we must preserve copyright notices when we adapt existing intellectual property.

I have no objection if you want to wait until the next iteration to fix the copyright notices, especially if this you are doing little more than carrying over existing code from P4-OVS in this iteration.

Incidentally, if any of the new files were adapted from existing code, you should add their copyright notices back in.

Sure, let me check about licenses one more time with Namrata and from legal standpoint as well, will take care of license in next commit

@ffoulkes
Copy link
Contributor

ffoulkes commented Aug 10, 2022

Sure, let me check about licenses one more time with Namrata and from legal standpoint as well, will take care of license in next commit

I'm talking about copyright notices, not licenses. They both go into the module header, but they're two different things, governed by different sets of laws.

Intel policy says that when we modify an existing work, we add our copyright notice below the existing ones.

By omitting the existing copyright notice, we appear to be claiming sole authorship of a work of which we are not the sole author. That's at best tacky, and at worst a violation of copyright law. Intel has a bad reputation in the open source community. We need to be punctilious in our observance, partly to maintain public good will, and partly because respect for intellectual property is an Intel value.

Barefoot Networks was the author at the time the work was created. Even though they're part of Intel now, their original declaration of authorship still stands.

This patch is initial commit to introduce linux
networking support in krnlmon.

Title: Add linux networking support
Signed-off-by: nupuruttarwar <nupur.uttarwar@intel.com>
@nupuruttarwar
Copy link
Contributor Author

Sure, let me check about licenses one more time with Namrata and from legal standpoint as well, will take care of license in next commit

I'm talking about copyright notices, not licenses. They both go into the module header, but they're two different things, governed by different sets of laws.

Intel policy says that when we modify an existing work, we add our copyright notice below the existing ones.

By omitting the existing copyright notice, we appear to be claiming sole authorship of a work of which we are not the sole author. That's at best tacky, and at worst a violation of copyright law. Intel has a bad reputation in the open source community. We need to be punctilious in our observance, partly to maintain public good will, and partly because respect for intellectual property is an Intel value.

Barefoot Networks was the author at the time the work was created. Even though they're part of Intel now, their original declaration of authorship still stands.

I agree, will make the changes in the next commit

@dandaly dandaly merged commit 99fa975 into ipdk-io:main Aug 10, 2022
vsureshkumarp pushed a commit that referenced this pull request Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants