Re: [Qemu-devel] [PATCH v3 2/3] block/backup: avoid copying less than full target clusters

[John Snow]

On 02/23/2016 07:45 PM, Eric Blake wrote:
> On 02/23/2016 05:27 PM, Fam Zheng wrote:
>> On Tue, 02/23 19:17, John Snow wrote:
>>> During incremental backups, if the target has a cluster size that is
>>> larger than the backup cluster size and we are backing up to a target
>>> that cannot (for whichever reason) pull clusters up from a backing image,
>>> we may inadvertantly create unusable incremental backup images.
>>>
> 
>>> +    /* If there is no backing file on the target, we cannot rely on COW if 
>>> our
>>> +     * backup cluster size is smaller than the target cluster size. Even 
>>> for
>>> +     * targets with a backing file, try to avoid COW if possible. */
>>> +    ret = bdrv_get_info(job->target, &bdi);
>>> +    if (ret < 0 && !target->backing) {
>>> +        error_setg_errno(errp, -ret,
>>> +            "Can't determine cluster size of target that has no backing 
>>> file. "
>>> +            "This may create an unusable destination image. Aborting.");
>>
>> Most error messages don't end with a period, but that's not a hard rule 
>> AFAICT.
> 
> Ah, but it is:
> 
> include/qapi/error.h:
> 
>  * The resulting message should be a single phrase, with no newline or
>  * trailing punctuation.
>  * Please don't error_setg(&error_fatal, ...), use error_report() and
>  * exit(), because that's more obvious.
>  * Likewise, don't error_setg(&error_abort, ...), use assert().
>  */
> #define error_setg(errp, fmt, ...)                              \
> 

":(.\n"

> The error message should be "Can't determine cluster size of target
> without backing file"; everything else should be done with
> error_append_hint(&errp, "Aborting, since this may create an unusable
> destination image\n")
> 
> 

Tch. I decided I don't like this message anyway.

"Couldn't determine the cluster size of the target image, which has no
backing file" is more correct. The problem is not why we couldn't
determine it, but instead that we were unable to AND there is no backing
file, so our inability to determine it is a problem.

What's our policy on error message strings that eclipse 80 columns? I
was told once (at gunpoint; I live in the USA) to never split up long
strings because it makes them harder to grep for.