Re: [Libguestfs] [PATCH nbdkit v2 10/10] tests: Test the Python plugin thoroughly.
On Sat, Nov 23, 2019 at 3:10 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:
On Sat, Nov 23, 2019 at 01:42:15AM +0200, Nir Soffer wrote:
> On Fri, Nov 22, 2019 at 9:55 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:
> > +def pread(h, count, offset, flags):
> > + assert flags == 0
> > + return h['disk'][offset:offset+count]
>
> Very nice and simple test plugin!
>
> But this returns always a bytearray, which is also what nbdkit python plugin
> expects. But real code using HTTPConnection return bytes:
>
> >>> c = http.client.HTTPSConnection("www.python.org")
> >>> c.request("GET", "/")
> >>> r = c.getresponse()
> >>> r.read()[:10]
> b'<!doctype '
>
> I think the plugin should support both bytearray, memoryview, or
> bytes. Supporting objects
> implementing the buffer protocol would be best.
I thought bytearray & bytes were the same thing :-/
Would adding this break existing nbdkit Python scripts? Should this
be considered for v2 of the API? Are there performance implications /
improvements from doing this?
There is performance implication for v1 plugins like rhv-upload-plugin that
need to copy the bytes from imageio server on python side:
r = http.getresponse()
...
return bytearray(r.read())
This is sad because on the C side we mempcpy the data again. So with this patch
we avoid one copy of the two.
To avoid all unneeded copies, we need to change pread() to:
def pread(h, buf, offset):
So the python side we can do:
f.readinto(buf)
Or:
sock.recv_info(buf)
It does not work for HTTPResponse, so in this case we have to do:
buf[:] = r.read()
Since we work on v2 now, I think we should consider this change.
An uglier alternative is:
def preadinto(h, buf, offset):
Matching python read() and readinto() interface.
> test-python-plugin.py
>
> This text can use python docstring:
>
> """
> This tests ...
> """
Good point, I'll fix this. We could probably also have nbdkit do
something with the docstring, such as printing it in --help output,
although that's something for another patch series.
> > + h = nbd.NBD ()
> > + cfg = codecs.encode (pickle.dumps (cfg), "base64").decode()
>
> base64.b64encode() is better, avoiding unwanted newlines.
Ah OK, I originally added strip(), but this is better.
> > + cmd = ["nbdkit", "-v", "-s",
"--exit-with-parent",
> > + "python", srcdir + "/test-python-plugin.py",
> > + "cfg=" + cfg]
> > + h.connect_command (cmd)
> > + return h
> > +
> > +# Test we can send an empty pickled test configuration and do nothing
> > +# else. This is just to ensure the machinery of the test works.
> > +h = test ({})
>
> So we have now running nbdkit that will exit the python collects when h
> is implicitly closed when creating a new handle?
>
> This is fragile, but can be solved with the help of a testing framework.
[...]
> pytest test-python.py
I'll probably use unittest though because it's built into Python and
because it's what we use in libguestfs, hivex etc but yes good idea.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW