Master Pull request for DPDK and Posix abstraction changes. (#152)
A message that is not in the archive (sent off-list or not yet received): spdk/spdk/pull/152/[email protected]
John Meneghini
Hi Ben.
I agree that commits fbed823 <https://github.com/spdk/spdk/commit/fbed8236facac9833bfd2677b7d76df79cc1c360>, 1bce206 <https://github.com/spdk/spdk/commit/1bce2064db31d44d900f41bab52764dcf65320e8>, edf25e7 <https://github.com/spdk/spdk/commit/edf25e716239ec0051df88207e6f70e474177839> can be dropped. These were the result of the fact that we are using -Wno-pointer-arith and -Wno-pointer-sign when compiling with clang-3.9.1. We’ve changed this. Commit 6ab852f <https://github.com/spdk/spdk/commit/6ab852f9e28d39dc021acbb9a0f0951e6dc933c0> was the result of running lint. We’ve disabled the process that runs lint on this code so you can drop that SHA as well.
I will review the new pull requests by the end of today.
John Meneghini
john.a.meneghini(a)gmail.com
Quoted 27 lines
Begin forwarded message:
From: Ben Walker <notifications(a)github.com>
Subject: Re: [spdk/spdk] Master Pull request for DPDK and Posix abstraction changes. (#152)
Date: May 3, 2017 at 2:37:14 PM EDT
To: spdk/spdk <spdk(a)noreply.github.com>
Cc: John Meneghini <john.a.meneghini(a)gmail.com>, Author <author(a)noreply.github.com>
Reply-To: spdk/spdk <reply+00c54bdda6bf8a3c64ce5c17da0be4a955707c78620de88e92cf000000011521e55a92a169ce0d7675a5(a)reply.github.com>I have so far picked off the following:
e2ecccb <https://github.com/spdk/spdk/commit/e2ecccb3423c01ac96a2073ab11c413f2e70f1bc> as pull request #153 <https://github.com/spdk/spdk/pull/153>
32f27bb <https://github.com/spdk/spdk/commit/32f27bb80026e73b73abd7e7f0eb83e9af81e68a> as pull request #154 <https://github.com/spdk/spdk/pull/154>
59072b9 <https://github.com/spdk/spdk/commit/59072b953699628d0447dcf8734473e26807bc26> as pull request #155 <https://github.com/spdk/spdk/pull/155>
Note that I made significant modifications to each of these commits as I picked them off. Please review on the relevant PRs.I also recommend we drop the following commits entirely:
fbed823 <https://github.com/spdk/spdk/commit/fbed8236facac9833bfd2677b7d76df79cc1c360>, 1bce206 <https://github.com/spdk/spdk/commit/1bce2064db31d44d900f41bab52764dcf65320e8>, edf25e7 <https://github.com/spdk/spdk/commit/edf25e716239ec0051df88207e6f70e474177839> - These are all adding casts but we're intentionally using the GNU C extension to treat void pointers asuint8_tpointers regarding pointer math. If you are unable to change your compiler settings to allow for this, let me know and we'll address it some other way.6ab852f <https://github.com/spdk/spdk/commit/6ab852f9e28d39dc021acbb9a0f0951e6dc933c0> - As far as we can see, this is a false positive from your linter and not an actual problem.
That accounts for 7 out of 31 of the commits so far. I'll continue to cherry pick and update here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <https://github.com/spdk/spdk/pull/152#issuecomment-298998284>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMVL3c0haArPauICl6fTTal_QsyvE6vQks5r2MlagaJpZM4NO4ku>.
John Meneghini
Also, with these changes we are trying to encourage more community participation.
I think there are many other individuals and companies in the SPDK community who have a stake in these change. As we go through this list of proposed patches (31 in all) I’d like to see some reviews and approvals from others in the community.
Can we agree upon a criteria for approval with changes?
I propose a criteria of at least 3 approvals for all commits:
1. from the proposing individual or representative company
2. from the Intel SPDK development team
3. from a third party
Thoughts?
John Meneghini
john.a.meneghini(a)gmail.com
Quoted 41 lines · John Meneghini
Hi Ben.
I agree that commits fbed823 <https://github.com/spdk/spdk/commit/fbed8236facac9833bfd2677b7d76df79cc1c360>, 1bce206 <https://github.com/spdk/spdk/commit/1bce2064db31d44d900f41bab52764dcf65320e8>, edf25e7 <https://github.com/spdk/spdk/commit/edf25e716239ec0051df88207e6f70e474177839> can be dropped. These were the result of the fact that we are using -Wno-pointer-arith and -Wno-pointer-sign when compiling with clang-3.9.1. We’ve changed this. Commit 6ab852f <https://github.com/spdk/spdk/commit/6ab852f9e28d39dc021acbb9a0f0951e6dc933c0> was the result of running lint. We’ve disabled the process that runs lint on this code so you can drop that SHA as well.
I will review the new pull requests by the end of today.
John Meneghini
john.a.meneghini(a)gmail.com <mailto:john.a.meneghini(a)gmail.com>Quoted 27 lines
Begin forwarded message:
From: Ben Walker <notifications(a)github.com <mailto:notifications(a)github.com>>
Subject: Re: [spdk/spdk] Master Pull request for DPDK and Posix abstraction changes. (#152)
Date: May 3, 2017 at 2:37:14 PM EDT
To: spdk/spdk <spdk(a)noreply.github.com <mailto:spdk(a)noreply.github.com>>
Cc: John Meneghini <john.a.meneghini(a)gmail.com <mailto:john.a.meneghini(a)gmail.com>>, Author <author(a)noreply.github.com <mailto:author(a)noreply.github.com>>
Reply-To: spdk/spdk <reply+00c54bdda6bf8a3c64ce5c17da0be4a955707c78620de88e92cf000000011521e55a92a169ce0d7675a5(a)reply.github.com <mailto:reply+00c54bdda6bf8a3c64ce5c17da0be4a955707c78620de88e92cf000000011521e55a92a169ce0d7675a5(a)reply.github.com>>I have so far picked off the following:
e2ecccb <https://github.com/spdk/spdk/commit/e2ecccb3423c01ac96a2073ab11c413f2e70f1bc> as pull request #153 <https://github.com/spdk/spdk/pull/153>
32f27bb <https://github.com/spdk/spdk/commit/32f27bb80026e73b73abd7e7f0eb83e9af81e68a> as pull request #154 <https://github.com/spdk/spdk/pull/154>
59072b9 <https://github.com/spdk/spdk/commit/59072b953699628d0447dcf8734473e26807bc26> as pull request #155 <https://github.com/spdk/spdk/pull/155>
Note that I made significant modifications to each of these commits as I picked them off. Please review on the relevant PRs.I also recommend we drop the following commits entirely:
fbed823 <https://github.com/spdk/spdk/commit/fbed8236facac9833bfd2677b7d76df79cc1c360>, 1bce206 <https://github.com/spdk/spdk/commit/1bce2064db31d44d900f41bab52764dcf65320e8>, edf25e7 <https://github.com/spdk/spdk/commit/edf25e716239ec0051df88207e6f70e474177839> - These are all adding casts but we're intentionally using the GNU C extension to treat void pointers asuint8_tpointers regarding pointer math. If you are unable to change your compiler settings to allow for this, let me know and we'll address it some other way.6ab852f <https://github.com/spdk/spdk/commit/6ab852f9e28d39dc021acbb9a0f0951e6dc933c0> - As far as we can see, this is a false positive from your linter and not an actual problem.
That accounts for 7 out of 31 of the commits so far. I'll continue to cherry pick and update here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <https://github.com/spdk/spdk/pull/152#issuecomment-298998284>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMVL3c0haArPauICl6fTTal_QsyvE6vQks5r2MlagaJpZM4NO4ku>.
Walker
Quoted 10 lines · John Meneghini
Also, with these changes we are trying to encourage more community participation.
I think there are many other individuals and companies in the SPDK community who have a stake in these change. As we go through this list of proposed patches (31 in all) I’d like to see some reviews and approvals from others in the community.
Can we agree upon a criteria for approval with changes?
Quoted 7 lines
I propose a criteria of at least 3 approvals for all commits:
1. from the proposing individual or representative company
2. from the Intel SPDK development team
3. from a third partyThoughts?
Feedback is always welcome from anyone. I encourage anyone with an opinion to post comments on the pull requests. We'll leave each request open for a few days minimum to solicit feedback.
The official process is that the SPDK maintainers (currently Daniel, Jim, and myself) can accept or deny a patch as we see fit. However, in practice that's going to mean that we've gathered feedback from all of the interested parties and we're confident that the patch meets everyone's needs. We won't ever just plow ahead without consensus unless we're not getting any participation.
For this particular series, we're not going to merge anything unless both you and Ranjit are ok with it (or provide no feedback for an extended period of time), since you two are the primary stakeholders in this effort. We're also going to try and poke a few people who have shown interest in this in the past and try to get some more feedback that way. If anyone else is interested in the DPDK/POSIX abstractions and starts commenting, we'll make sure their feedback is all incorporated into the final patches as well.