Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

On 03/06/2017 02:14 PM, Max Reitz wrote:

>>>      if (S_ISREG(st.st_mode)) {
>>>          if (ftruncate(s->fd, offset) < 0) {
>>> +            /* The generic error message will be fine */
>>>              return -errno;
>>
>> Relying on a generic error message in the caller is awkward. I see it as
>> evidence of a partial conversion if we have an interface that requires a
>> return of a negative errno value to make up for when errp is not set.
> 
> I'm not sure what you mean by this exactly.

The ideal conversion would be having .bdrv_truncate switch to a void
return; then no caller is messing with negative errno values (especially
since some of them are just synthesizing errno values, rather than
actually encountering one, and since some of them are probably using -1
when they should be using errno). But such a conversion requires that
all drivers be updated to properly set errp.


> I can agree that it's not
> nice within a single driver. But I think it's fine to have some drivers
> that generate an Error object and others that do not, as long as the
> generic interface (bdrv_truncate()) masks this behavior.

I agree that as an interim thing, having some drivers that aren't
converted yet is the best way to make progress. But it's still best if
every driver is switched on an all-or-none basis, so that we don't
forget to go back and re-fix drivers that half-heartedly set errp if we
eventually reach the point where all other drivers have been fixed.

In other words, I view the generic bdrv_truncate() supplying an error
based on negative errno is a crutch, and not something that we should
rely on, at least not for the drivers that we fix to set errp directly.

> 
>>                                                                        I
>> know you aren't comfortable converting all drivers, but for the drivers
>> you do convert, I'd rather guarantee that ALL errors set errp.
> 
> Can do, I just felt bad that it be would exactly the same as the message
> that would be generated in bdrv_truncate() anyway.

There may be some duplication of error messages, but it still seems
cleaner to consistently set the error within the driver than to rely on
the generic crutch.

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

signature.asc
Description: OpenPGP digital signature