[libvirt] [PATCH v2 3/5] Add a helper API for setting up a NBD device with qemu-nbd

[Daniel P. Berrange]

On Tue, Apr 30, 2013 at 05:55:15PM -0600, Eric Blake wrote:
> On 04/22/2013 08:06 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > Add a virFileNBDDeviceAssociate method, which given a filename
> > will setup a NBD device, using qemu-nbd as the server.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  src/libvirt_private.syms |   1 +
> >  src/util/virfile.c       | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  src/util/virfile.h       |   6 ++
> >  3 files changed, 155 insertions(+), 3 deletions(-)
> > 
> 
> > +++ b/src/util/virfile.c
> > @@ -530,6 +530,7 @@ static int virFileLoopDeviceOpen(char **dev_name)
> >          goto cleanup;
> >      }
> >  
> > +    errno = 0;
> >      while ((de = readdir(dh)) != NULL) {
> >          if (!STRPREFIX(de->d_name, "loop"))
> >              continue;
> > @@ -561,10 +562,15 @@ static int virFileLoopDeviceOpen(char **dev_name)
> >          /* Oh well, try the next device */
> >          VIR_FORCE_CLOSE(fd);
> >          VIR_FREE(looppath);
> > +        errno = 0;
> >      }
> >  
> > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -                   _("Unable to find a free loop device in /dev"));
> > +    if (errno != 0)
> > +        virReportSystemError(errno, "%s",
> > +                             _("Unable to iterate over loop devices"));
> > +    else
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Unable to find a free loop device in /dev"));
> 
> Hmm, this looks like an independent (but useful) cleanup.
> 
> 
> > +static char *
> > +virFileNBDDeviceFindUnused(void)
> > +{
> > +    DIR *dh;
> > +    char *ret = NULL;
> > +    struct dirent *de;
> > +
> > +    if (!(dh = opendir(SYSFS_BLOCK_DIR))) {
> > +        virReportSystemError(errno,
> > +                             _("Cannot read directory %s"),
> > +                             SYSFS_BLOCK_DIR);
> > +        return NULL;
> > +    }
> > +
> > +    while ((de = readdir(dh)) != NULL) {
> 
> Oops, you're missing errno checking here (readdir can be such a pain to
> use correctly).
> 
> > +    cmd = virCommandNew(qemunbd);
> > +
> > +    /* Explicitly not trying to cope with old qemu-nbd which
> > +     * lacked --format. We want to see a fatal error in that
> > +     * case since it would be security flaw to continue */
> > +    if (fmtstr) {
> > +        virCommandAddArg(cmd, "--format");
> > +        virCommandAddArg(cmd, fmtstr);
> > +    }
> 
> You could shorten if you wanted:
> 
> if (fmtstr)
>     virCommandAddArgList(cmd, "--format", fmtstr, NULL);
> 
> 
> >  #else /* __linux__ */
> >  
> >  int virFileLoopDeviceAssociate(const char *file,
> > -                               char **dev ATTRIBUTE_UNUSED)
> > +                               char **dev ATTRIBUTE)
> 
> Whoa - that can't be right.
> 
> The idea is good, but the patch isn't quite perfect.  Are you still
> shooting for 1.0.5?

No, we're too late into the freeze to consider this now IMHO


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|