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

On 04/22/2013 08:06 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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(a)redhat.com&gt; > --- > 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?