[libvirt] [PATCH 4/7] Add a helper API for setting up a NBD device with qemu-nbd

[Eric Blake]

On 03/19/2013 07:39 AM, John Ferlan wrote:
> On 03/15/2013 12:32 PM, 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>
>> ---

>> +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) {

readdir() is an annoying interface.  To properly distinguish errors, you
have to assign errno to 0 before each iteration, then check if errno is
set when the while loop terminates.

>> +        if (STRPREFIX(de->d_name, "nbd")) {
>> +            int rv = virFileNBDDeviceIsBusy(de->d_name);
>> +            if (rv < 0)
>> +                goto cleanup;
>> +            if (rv == 0) {
>> +                if (virAsprintf(&ret, "/dev/%s", de->d_name) < 0) {
>> +                    virReportOOMError();
>> +                    goto cleanup;
>> +                }
>> +                goto cleanup;
>> +            }
>> +        }
>> +    }
>> +
>> +    virReportSystemError(EBUSY, "%s",
>> +                         _("No free NBD devices"));

Your choice of error reporting is to blindly overwrite any (potential)
error from readdir.  That potentially loses information.  Then again,
since you normally succeed only by finding a winning existing entry, and
since you are at least giving a reasonable error message, even if you
overwrote the readdir() errno, I can live with it.

>> +int virFileNBDDeviceAssociate(const char *file,
>> +                              char **dev ATTRIBUTE_UNUSED,
>> +                              bool readonly ATTRIBUTE_UNUSED)
>> +{
>> +    virReportSystemError(ENOSYS,
>> +                         _("Unable to associate file %s with NBD device"),
>> +                         file);
>> +    *dev = NULL;
> 
> Since this is done - should this still be UNUSED in the header?

It doesn't hurt to leave it in, but it also doesn't hurt to take out the
ATTRIBUTE_UNUSED or even drop the '*dev = NULL' line.

> ACK  in any case.

Agreed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130320/9cfbb0b9/attachment-0001.sig>