Re: [PATCH] Add optional nbd:// io space support

[Jose E. Marchesi]

Hi Eric.

    This uses libnbd to access any NBD server as an io space.  Tests will
    come in the next patch.
    
    * configure.ac (PKG_CHECK_MODULES): Check for libnbd.
    * src/ios-dev-nbd.c: New file.
    * src/Makefile.am (poke_SOURCES): Optionally build it.
    * src/ios.c (ios_dev_ifs): Expose it through 'open'.
    * src/pk-ios.c (pk_cmd_nbd): Add .nbd command.
    * src/pk-cmd.c (dot_cmds): Load it.
    * doc/poke.texi (nbd command): New section.
    (open): Document nbd handler.
    * HACKING (libnbd): Mention it.

Very nice! :)
Please see a few comments below.

    diff --git a/configure.ac b/configure.ac
    index 5e27d01a..7fd21f39 100644
    --- a/configure.ac
    +++ b/configure.ac
    @@ -82,6 +82,17 @@ dnl Jitter
    
     AC_JITTER_SUBPACKAGE([jitter])
    
    +dnl libnbd for nbd:// io spaces (optional)
    +PKG_CHECK_MODULES([LIBNBD], [libnbd], [
    +  AC_SUBST([LIBNBD_CFLAGS])
    +  AC_SUBST([LIBNBD_LIBS])
    +  AC_DEFINE([HAVE_LIBNBD], [1], [libnbd found at compile time])
    +  libnbd_enabled=yes
    +], [libnbd_enabled=no
    +  AC_MSG_WARN([libnbd not found, nbd:// io spaces will be disabled.])

I would prefer for this warning to be printed at the end of configure.ac
along with the other warning related to missing optional features.
Otherwise it is too easy to miss it.

    +])
    +AM_CONDITIONAL([NBD], [test "x$libnbd_enabled" = "xyes"])
    +
     dnl Used in Makefile.am.  See the note there.
     WITH_JITTER=$with_jitter
     AC_SUBST([WITH_JITTER])

    diff --git a/src/pk-ios.c b/src/pk-ios.c
    index 08ca89f8..361fd366 100644
    --- a/src/pk-ios.c
    +++ b/src/pk-ios.c
    @@ -330,6 +330,45 @@ pk_cmd_mem (int argc, struct pk_cmd_arg argv[], 
uint64_t uflags)
         return 1;
     }
    
    +static int
    +pk_cmd_nbd (int argc, struct pk_cmd_arg argv[], uint64_t uflags)
    +{
    +#ifdef HAVE_LIBNBD
    +  /* nbd URI */
    +
    +  assert (argc == 1);
    +  assert (PK_CMD_ARG_TYPE (argv[0]) == PK_CMD_ARG_STR);
    +
    +  /* Create a new NBD IO space.  */
    +  const char *arg_str = PK_CMD_ARG_STR (argv[0]);
    +  char *nbd_name = xstrdup (arg_str);
    +
    +  if (ios_search (nbd_name) != NULL)
    +    {
    +      printf (_("Buffer %s already opened.  Use `.ios #N' to switch.\n"),
    +              nbd_name);
    +      free (nbd_name);
    +      return 0;
    +    }
    +
    +  if (IOS_ERROR == ios_open (nbd_name, 0, 1))
    +    {
    +      pk_printf (_("Error creating NBD IOS %s\n"), nbd_name);
    +      free (nbd_name);
    +      return 0;
    +    }
    +
    +  if (poke_interactive_p && !poke_quiet_p)
    +    pk_printf (_("The current IOS is now `%s'.\n"),
    +               ios_handler (ios_cur ()));
    +
    +  return 1;
    +#else /* ! HAVE_LIBNBD */
    +  pk_printf (_("This poke was compiled without NBD support\n"));

Hm, what about conditionally compile the whole pk_cmd_nbd function (and
related structures like nbd_cmd) so we don't need this message?

    +  return 0;
    +#endif
    +}
    +
     struct pk_cmd ios_cmd =
       {"ios", "t", "", 0, NULL, pk_cmd_ios, "ios #ID", 
ios_completion_function};
    
Other than the above, it looks great! :)
Thanks.