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

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(a)redhat.com&gt; 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/5a02c7d2cc6a201f9e5531c0c20c2f3... > > > > > > 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?