Re: [libvirt] [PATCH 5/5]: Rewrite findLuns function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jim Meyering wrote:
>> diff -urp libvirt.sendtarget/src/storage_backend_iscsi.c libvirt.findLun/src/storage_backend_iscsi.c
>> --- libvirt.sendtarget/src/storage_backend_iscsi.c	2008-06-16 14:35:34.000000000 +0200
>> +++ libvirt.findLun/src/storage_backend_iscsi.c	2008-06-16 14:52:31.000000000 +0200
>> @@ -119,7 +119,7 @@ virStorageBackendISCSISession(virConnect
> ...
>> +    while ((sys_dirent = readdir(sysdir))) {
>> +        /* double-negative, so we can use the same function for scandir below */
>> +        if (!notdotdir(sys_dirent))
>> +            continue;
>> +
>> +        if (STREQLEN(sys_dirent->d_name, "target", 6)) {
>> +            if (sscanf(sys_dirent->d_name, "target%u:%u:%u",
>> +                       &host, &bus, &target) != 3) {
>> +                virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
>> +                                      _("Failed to parse target in sysfs path %s: %s"),
>> +                                      sysfs_path, strerror(errno));
> 
> You can remove the ": %s" suffix and the strerror(errno) argument,
> since errno isn't relevant here.  Or maybe better: leave the suffix
> and use sys_dirent->d_name as the argument, so the diagnostic shows
> what couldn't be parsed.

Oops!  Cut and pasted that from elsewhere, and didn't fix up the error messages.
 I'll fix that up as you advise.

> 
>> +                closedir(sysdir);
>> +                return -1;
>> +				
> 
> The above line has just three TABs.
> Best to delete it.

Yeah, probably leftover from my whitespace fixup.  I'll fix that as well.

> 
> The rest, including patches 1-4, looks fine,
> so, pending whatever tests you deem adequate,
> ACK.

Thanks for the review!

Chris Lalancette

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]