New Rust bindings for nbdkit
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and they are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most likely, nobody has tried to use them yet, since the crate hasn't even published to crates.io. Please review the attached patch. -Alan
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and they are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most likely, nobody has tried to use them yet, since the crate hasn't even published to crates.io. Please review the attached patch. -Alan
Thanks Alan, and sorry for the delayed reply. It just happened that this email arrived before a long weekend. First a note that it was a bit of a manual process for me to apply this because I think it was prepared using "git diff"(?) If you use "git format-patch" or (better) "git send-email" for future patches then that's much easier. However I was able to apply and review it. I have pushed it, but added a few changes which I will summarise below: * Some lines had trailing whitespace, removed. These are only allowed in POD where it is used for verbatim sections. * In VERSION docs we usually refer only to stable (even-numbered) releases, so I replaced 1.21.9 -> 1.22. * Remove plugins/rust/Cargo.toml from .gitignore. * Several newly added files were missing from EXTRA_DIST (make && make dist && make maintainer-check-extra-dist will find these in future). This will create small rebase problems for you if you've made further changes, but hopefully nothing that isn't easily fixable. Other issues: * The license removed this clause: -// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission. I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us. * Although the build works (or doesn't break), make check doesn't appear to run the tests. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Mon, Jun 15, 2020 at 7:22 AM Richard W.M. Jones <rjones@redhat.com> wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote: they
are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most likely, nobody has tried to use them yet, since the crate hasn't even published to crates.io. Please review the attached patch. -Alan
Thanks Alan, and sorry for the delayed reply. It just happened that this email arrived before a long weekend.
First a note that it was a bit of a manual process for me to apply this because I think it was prepared using "git diff"(?) If you use "git format-patch" or (better) "git send-email" for future patches then that's much easier. However I was able to apply and review it.
I have pushed it, but added a few changes which I will summarise below:
* Some lines had trailing whitespace, removed. These are only allowed in POD where it is used for verbatim sections.
* In VERSION docs we usually refer only to stable (even-numbered) releases, so I replaced 1.21.9 -> 1.22.
* Remove plugins/rust/Cargo.toml from .gitignore.
* Several newly added files were missing from EXTRA_DIST (make && make dist && make maintainer-check-extra-dist will find these in future).
This will create small rebase problems for you if you've made further changes, but hopefully nothing that isn't easily fixable.
Other issues:
* The license removed this clause:
-// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission.
I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us.
* Although the build works (or doesn't break), make check doesn't appear to run the tests.
Rich.
Ok, I can make those changes. Speaking of "make check", what about CI? It would be pretty easy to add it to this project. Is there a reason you haven't done it already? -Alan
On Mon, Jun 15, 2020 at 07:42:58AM -0600, Alan Somers wrote:
Ok, I can make those changes. Speaking of "make check", what about CI? It would be pretty easy to add it to this project. Is there a reason you haven't done it already?
We're probably going to move to an open source hosting service at some point, and don't want to depend too deeply on closed source solutions like github. At the moment I'm running the tests regularly but we'll implement CI at some point. (It's fine for others to run the tests on their own systems and report the results.) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Mon, Jun 15, 2020 at 02:22:32PM +0100, Richard W.M. Jones wrote:
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and they are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most likely, nobody has tried to use them yet, since the crate hasn't even published to crates.io. Please review the attached patch. -Alan
Other issues:
* The license removed this clause:
-// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission.
I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us.
This change is replacing 3-clause BSD with 2-clause BSD. Shouldn't cuase any actual difference for consumers, but seems like a needless change to be making. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jun 15, 2020 at 8:04 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and
are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most
nobody has tried to use them yet, since the crate hasn't even
On Mon, Jun 15, 2020 at 02:22:32PM +0100, Richard W.M. Jones wrote: they likely, published to
crates.io. Please review the attached patch. -Alan
Other issues:
* The license removed this clause:
-// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission.
I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us.
This change is replacing 3-clause BSD with 2-clause BSD. Shouldn't cuase any actual difference for consumers, but seems like a needless change to be making.
Regards, Daniel
My reasoning was that the 2-clause license is preferred for new code (at least in every other community where I've active), and this plugin is new code. But I can add the 3rd clause back if libguestfs likes it. -Alan
On Mon, Jun 15, 2020 at 08:11:47AM -0600, alan somers wrote:
On Mon, Jun 15, 2020 at 8:04 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and
are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most
nobody has tried to use them yet, since the crate hasn't even
On Mon, Jun 15, 2020 at 02:22:32PM +0100, Richard W.M. Jones wrote: they likely, published to
crates.io. Please review the attached patch. -Alan
Other issues:
* The license removed this clause:
-// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission.
I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us.
This change is replacing 3-clause BSD with 2-clause BSD. Shouldn't cuase any actual difference for consumers, but seems like a needless change to be making.
My reasoning was that the 2-clause license is preferred for new code (at least in every other community where I've active), and this plugin is new code. But I can add the 3rd clause back if libguestfs likes it.
I don't have any opinion on which variant is better. The important question is whether this new Rust impl was a completely clean room impl, or whether it started from existing code & then adapted it. If the latter, then changing the license is bad practice, as is removing existing Copyright statements. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jun 15, 2020 at 8:30 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Mon, Jun 15, 2020 at 08:11:47AM -0600, alan somers wrote:
On Mon, Jun 15, 2020 at 8:04 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and
are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most
nobody has tried to use them yet, since the crate hasn't even
On Mon, Jun 15, 2020 at 02:22:32PM +0100, Richard W.M. Jones wrote: they likely, published to
crates.io. Please review the attached patch. -Alan
Other issues:
* The license removed this clause:
-// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission.
I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us.
This change is replacing 3-clause BSD with 2-clause BSD. Shouldn't cuase any actual difference for consumers, but seems like a needless change to be making.
My reasoning was that the 2-clause license is preferred for new code (at least in every other community where I've active), and this plugin is new code. But I can add the 3rd clause back if libguestfs likes it.
I don't have any opinion on which variant is better.
The important question is whether this new Rust impl was a completely clean room impl, or whether it started from existing code & then adapted it. If the latter, then changing the license is bad practice, as is removing existing Copyright statements.
Regards, Daniel
It's not a full clean-room implemention like Compaq's BIOS; I did read the original. However, the new plugin contains nothing of the original except for fragments of struct definitions. -Alan
On Mon, Jun 15, 2020 at 08:11:47AM -0600, alan somers wrote:
On Mon, Jun 15, 2020 at 8:04 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Thu, Jun 11, 2020 at 04:19:08PM -0600, alan somers wrote:
The existing Rust bindings for nbdkit aren't very idiomatic Rust, and
are missing a lot of features. So I've rewritten them. The new bindings aren't backwards compatible, but I doubt that's a problem. Most
nobody has tried to use them yet, since the crate hasn't even
On Mon, Jun 15, 2020 at 02:22:32PM +0100, Richard W.M. Jones wrote: they likely, published to
crates.io. Please review the attached patch. -Alan
Other issues:
* The license removed this clause:
-// * Neither the name of Red Hat nor the names of its contributors may be -// used to endorse or promote products derived from this software without -// specific prior written permission.
I believe this removal simply makes the license even more permissive, so that's fine. However I will check with our legal people. Also you should add license headers to the new files plugins/rust/tests/*.rs. Essentially every file should have a license, and correct licensing is very important to us.
This change is replacing 3-clause BSD with 2-clause BSD. Shouldn't cuase any actual difference for consumers, but seems like a needless change to be making.
Regards, Daniel
My reasoning was that the 2-clause license is preferred for new code (at least in every other community where I've active), and this plugin is new code. But I can add the 3rd clause back if libguestfs likes it.
I'm not really fussed about this one either way. Much more important are the files which lack license boilerplate at the top. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
participants (4)
-
Alan Somers -
alan somers -
Daniel P. Berrangé -
Richard W.M. Jones