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

Re: [libvirt] [PATCH 1/3] storage: mpath: Clean up some error handling



On 05/20/2010 06:10 PM, Eric Blake wrote:
> On 05/20/2010 01:23 PM, Cole Robinson wrote:
>> We were squashing error messages in a few cases. Recode to follow common
>> ret = -1 convention.
> 
> s/squashing/duplicating/, but I did check that the only caller,
> virStorageBackendMpathNewVol does print a message when it gets a
> negative result.
> 

The error squashing I was refering to here was of
BackendUpdateVolTargetInfoFD.

>> @@ -43,39 +43,27 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target,
>>                                            unsigned long long *allocation,
>>                                            unsigned long long *capacity)
>>  {
>> -    int ret = 0;
>> +    int ret = -1;
>>      int fd = -1;
>>  
>>      if ((fd = open(target->path, O_RDONLY)) < 0) {
>>          virReportSystemError(errno,
>>                               _("cannot open volume '%s'"),
>>                               target->path);
>> -        ret = -1;
>>          goto out;
> 
> Unfortunately, this means that we lose errno; if open fails, we get just
> the less-specific message:
> 
>         virStorageReportError(VIR_ERR_INTERNAL_ERROR,
>                               _("Failed to update volume for '%s'"),
>                               vol->target.path);
>

Ah, I see, I didn't notice the squashing going on in MpathNewVol.

> Would it make more sense to refactor this in the other direction, to
> make virStorageBackendMpathUpdateVolTargetInfo always print the error
> message on failure, where we still have errno, and make
> virStorageBackendMpathNewVol silent instead of duplicating the message?
> 

A failing function's error messages should not be overwritten, so
MpathNewVol is wrong here. Thanks for pointing that out, I'll update and
resend.

Thanks,
Cole


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