[libvirt] [PATCH 02/18] Fix error detection in virStorageBackendISCSIGetHostNumber

[Ján Tomko]

On Thu, Jun 23, 2016 at 08:26:06AM -0400, John Ferlan wrote:
>
>
>On 06/21/2016 12:05 PM, Ján Tomko wrote:
>> In the unlikely case the iSCSI session path exists, but does not
>> contain an entry starting with "target", we would silently use
>> an initialized value.
>>
>> Rewrite the function to correctly report errors.
>> ---
>>  src/storage/storage_backend_iscsi.c | 38 +++++++++++++++++++------------------
>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
>> index bccfba3..876c14c 100644
>> --- a/src/storage/storage_backend_iscsi.c
>> +++ b/src/storage/storage_backend_iscsi.c
>> @@ -90,7 +90,7 @@ static int
>>  virStorageBackendISCSIGetHostNumber(const char *sysfs_path,
>>                                      uint32_t *host)
>>  {
>> -    int retval = 0;
>> +    int ret = -1;
>>      DIR *sysdir = NULL;
>>      struct dirent *dirent = NULL;
>>      int direrr;
>> @@ -104,26 +104,33 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path,
>>      if (sysdir == NULL) {
>>          virReportSystemError(errno,
>>                               _("Failed to opendir path '%s'"), sysfs_path);
>> -        retval = -1;
>> -        goto out;
>> +        goto cleanup;
>>      }
>>
>>      while ((direrr = virDirRead(sysdir, &dirent, sysfs_path)) > 0) {
>>          if (STREQLEN(dirent->d_name, "target", strlen("target"))) {
>
>While we're here and changing, could use STRPREFIX
>

I'll send a separate patch for that.

>> -            if (sscanf(dirent->d_name,
>> -                       "target%u:", host) != 1) {
>> -                VIR_DEBUG("Failed to parse target '%s'", dirent->d_name);
>> -                retval = -1;
>> -                break;
>> +            if (sscanf(dirent->d_name, "target%u:", host) == 1) {
>> +                ret = 0;
>> +                goto cleanup;
>> +            } else {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("Failed to parse target '%s'"), dirent->d_name);
>> +                goto cleanup;
>>              }
>>          }
>>      }
>> -    if (direrr < 0)
>> -        retval = -1;
>>
>> +    if (direrr == 0) {
>
>Should this be <= ?
>
>If virDirRead() returns -1, then we don't return with an error...
>

virDirRead does report errors.

>If virDirRead() returns 0, then we've gone through the entire directory
>without finding an entry that starts with target - seems that would be a
>system configuration error and I assume errno would be ENOENT
>

Actually, errno would probably be 0. I'll fix the error in v2.

Jan

>ACK with obvious adjustments -
>
>John