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