On Thu, Oct 5, 2023, 10:05 AM Eric Blake <ebl...@redhat.com> wrote:
> On Wed, Oct 04, 2023 at 03:46:05PM -0400, John Snow wrote:
> > The test bails gracefully if this module isn't installed, but linters
> > need a little help understanding that. It's enough to just declare the
> > type in this case.
> >
> > (Fixes pylint complaining about use of an uninitialized variable because
> > it isn't wise enough to understand the notrun call is noreturn.)
> >
> > Signed-off-by: John Snow <js...@redhat.com>
> > ---
> > tests/qemu-iotests/tests/nbd-multiconn | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Since there's questions about this pull request seeming to be the
> first time this patch has appeared on list, I'll go ahead and review
> it here on the assumption that a v2 pull request is warranted.
>
Sorry... Tried to "sneak" in some real trivial stuff, but didn't mean to
try and pull a fast one. I figured it'd get reviewed and then we'd merge. I
can see this sentiment is not a well expected one 😓
> >
> > diff --git a/tests/qemu-iotests/tests/nbd-multiconn
> b/tests/qemu-iotests/tests/nbd-multiconn
> > index 478a1eaba27..7e686a786ea 100755
> > --- a/tests/qemu-iotests/tests/nbd-multiconn
> > +++ b/tests/qemu-iotests/tests/nbd-multiconn
> > @@ -20,6 +20,8 @@
> >
> > import os
> > from contextlib import contextmanager
> > +from types import ModuleType
> > +
> > import iotests
> > from iotests import qemu_img_create, qemu_io
> >
> > @@ -28,7 +30,7 @@ disk = os.path.join(iotests.test_dir, 'disk')
> > size = '4M'
> > nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')
> > nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock
> > -
> > +nbd: ModuleType
>
> Is it possible to put this closer to the code actually using 'nbd', as
> in this region?
>
> | if __name__ == '__main__':
> | try:
> | # Easier to use libnbd than to try and set up parallel
> | # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems
> | # have libnbd installed.
> | import nbd # type: ignore
>
> but then again, open_nbd() right after your current location utilizes
> the variable, so I guess not. I trust your judgment on silencing the
> linters, so whether or not you move it (if moving is even possible),
>
It might be possible to move things around to be more localized, but this
was the smallest possible diffstat.
It's not really about the type, the declaration also helps pylint not
complain the "potentially" illegal use. (type: ignore isn't sufficient.)
The alternative is, I think, using some try...except around the import up
at the top, and using a HAVE_NBDLIB variable that we use to exit early
instead. I think there's no real benefit to that much churn, and this gets
the job done with less.
It might be possible to teach pylint that notrun is a NORETURN function,
but I didn't explore that avenue.
> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
Thanks, and apologies for the fastball.
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
>
