On Sun, Nov 24, 2019, 12:42 Richard W.M. Jones <rjones@redhat.com> wrote:
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@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@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.

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?

We keep nicer pread for compatibility and add preadinto for people that care about performence. This is the current approach in the standard library.


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