[Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 10:41:46AM -0600, Eric Blake wrote:
> On Tue, Jan 31, 2023 at 11:52:27AM +0000, Richard W.M. Jones wrote:
> > On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
> > > On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones <rjones at redhat.com> wrote:
> > > >
> > > > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > > > > Document all options in --help output.  If -n is not in use, then
> > > > > enhance the banner to print the current state of h, and further tailor
> > > > > the advice given on useful next steps to take to mention opt_go when
> > > > > using --opt-mode.
> > > >
> > > > I had to partially revert this patch (reverting most of it) because it
> > > > unfortunately breaks the implicit handle creation :-(
> > > >
> > > > https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2
> > > >
> > > > I'm not actually sure how to do this correctly in Python.  I made
> > > > several attempts, but I don't think Python is very good about having a
> > > > variable which is only defined on some paths -- maybe it's not
> > > > possible at all.
> > > 
> > > Can you share the error when it breaks?
> > 
> > $ rpm -qf /usr/bin/nbdsh
> > python3-libnbd-1.15.9-2.fc38.x86_64
> > 
> > $ nbdsh 
> > 
> > Welcome to nbdsh, the shell for interacting with
> > Network Block Device (NBD) servers.
> > 
> > The ‘nbd’ module has already been imported and there
> > is an open NBD handle called ‘h’ in state 'START'.
> > 
> > h.connect_tcp("remote", "10809")   # Connect to a remote server.
> > h.get_size()                       # Get size of the remote disk.
> > buf = h.pread(512, 0)              # Read the first sector.
> > exit() or Ctrl-D                   # Quit the shell
> > help(nbd)                          # Display documentation
> > 
> > nbd> print(h)
> > Traceback (most recent call last):
> >   File "/usr/lib64/python3.11/code.py", line 90, in runcode
> >     exec(code, self.locals)
> >   File "<console>", line 1, in <module>
> > NameError: name 'h' is not defined
> 
> Eww. I didn't mean for that to happen, obviously.
> 
> > 
> > > I'm not sure what is the issue, but usually if you have a global
> > > variable created only in
> > > some flows, adding:
> > > 
> > >     thing = None
> > > 
> > > At the start of the module makes sure that the name exists later,
> > > regardless of the flow
> > > taken. Code can take the right action based on:
> > > 
> > >     if thing is None:
> > >         ...
> > 
> > Good point, I was trying 'undef h' instead.  'h' not being present in
> > the dictionary at all vs 'h = None' are slightly different, although I
> > suppose it doesn't matter in this particular case.
> > 
> > The other point is that 'h' (when defined) is not a global.  The patch
> > assumes it is a global, but then uses it in some places as if it is a
> > local.
> 
> Do you want me to try to play with this as well (since I do like the
> improved help message, if we can get it to work reliably), or would I
> be duplicating efforts at this point?

I don't have any further plans here, so if you want to.  However I
would say it's very subtle.  In particular I don't have a clear mental
model of how variable scope works in Python.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v