Libguestfs - Nov 2019 - Re: [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@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.

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
>

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?

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

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