Re: [Libguestfs] [PATCH nbdkit v2 10/10] tests: Test the Python plugin thoroughly.

On Sat, Nov 23, 2019 at 06:11:47PM +0200, Nir Soffer wrote: > On Sat, Nov 23, 2019 at 3:10 PM Richard W.M. Jones <rjones(a)redhat.com&gt; 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&gt; 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. Right, we can consider this for v2, while leaving v1 callers alone. > An uglier alternative is: > > def preadinto(h, buf, offset): > > Matching python read() and readinto() interface. Is this different somehow from def pread(h, buf, offset) defined above?

Rich. > > > 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 > > -- 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