Re: [PATCH v2] nbd/server: Allow MULTI_CONN for shared writable exports

[Eric Blake]

On Wed, Feb 16, 2022 at 07:14:58PM +0200, Nir Soffer wrote:
> > > I'm not the best at writing python iotests; I welcome a language
> > > translation of this aspect.
> >
> >
> >
> > Let me try:)
> 
> Thanks! This is much nicer and will be easier to maintain.
> 
> >
> >
> > #!/usr/bin/env python3
> >
> > import os
> > import iotests
> > import nbd

What happens here if libnbd is not installed?  Is there a way to make
the test gracefully skip instead of error out?

> >      def check_multi_conn(self, export_name, multi_conn):
> >          cl = nbd.NBD()
> >          cl.connect_uri(nbd_uri.format(export_name))
> >          self.assertEqual(cl.can_multi_conn(), multi_conn)
> >          cl.shutdown()
> 
> The test will be more clear and the code more reusable if the helper
> was doing only connect/disconnect.
> 
> @contextmanager
> def open_nbd(nbd_uri, export_name):
>      h = nbd.NBD()
>      h.connect_uri(nbd_uri.format(export_name))

This can throw an exception if the connect fails; should it be inside
the try?  I'm comparing to libnbd/python/t/220-opt-list.py, which also
sets up a context manager.

>      try:
>          yield h
>      finally:
>          h.shutdown()
> 
> Any test that need nbd handle can do:
> 
>     with open_nbd(nbd_uri, export_name) as h:
>         use the handle...
> 
> Good example when we can use this is the block status cache test,
> using complicated qemu-img commands instead of opening
> a client and calling block_status a few times.
> 
> And this also cleans up at the end of the test so failure
> will not affect the next test.
> 
> The helper can live in the iotest module instead of inventing it for
> every new test.

Moving it into iotest makes the question about what to do if libnbd is
not installed even more important; while we could perhaps catch and
deal with a failed 'import' for this test, skipping the iotest module
due to a failed import has knock-on effects to all other iotests even
when they don't care if libnbd is installed.

> 
> >
> >      def test_default_settings(self):
> >          self.server_start()
> >          self.export_add('r'))
> >          self.export_add('w', writable=True)
> >          self.check_multi_conn('r', True)
> >          self.check_multi_conn('w', False)
> 
> With the helper suggested, this test will be:
> 
>     with open_nbd(nbd_uri, "r") as h:
>         self.assertTrue(h.can_multi_conn())
> 
>     with open_nbd(nbd_uri, "w") as h:
>         self.assertFalse(h.can_multi_conn())
> 
> Now you don't need to check what check_multicon() is doing when
> reading the tests, and it is very clear what open_nbd() does based
> on the name and usage as context manager.

Yes, I like that aspect of a context manager.

> 
> >
> >      def test_multiconn_option(self):
> >          self.server_start()
> >          self.export_add('r', multi_conn='off')
> >          self.export_add('w', writable=True, multi_conn='on')
> 
> It will be more natural to use:
> 
>     self.start_server()
>     self.add_export(...)
> 
> In C the other way is more natural because you don't have namespaces
> or classes.
> 
> >          self.check_multi_conn('r', False)
> >          self.check_multi_conn('w', True)
> 
> And I think you agree since you did not use:
> 
>     self.multi_con_check(...)
>

Useful naming advice.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org