Re: [Qemu-devel] [PATCH v2 01/12] block: add error parameter to bdrv_snapshot_create() and related functions

[Pavel Hrdina]

On 26.3.2013 15:22, Eric Blake wrote:
> On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <address@hidden>
> > ---
> >   block.c                   | 22 ++++++++++++++++------
> >   block/qcow2-snapshot.c    |  9 ++++++++-
> >   block/qcow2.h             |  4 +++-
> >   block/rbd.c               |  8 ++++++--
> >   block/sheepdog.c          | 17 +++++++++--------
> >   include/block/block.h     |  3 ++-
> >   include/block/block_int.h |  3 ++-
> >   qemu-img.c                |  2 +-
> >   savevm.c                  |  2 +-
> >   9 files changed, 48 insertions(+), 22 deletions(-)
>
> >   int bdrv_snapshot_create(BlockDriverState *bs,
> > -                         QEMUSnapshotInfo *sn_info)
> > +                         QEMUSnapshotInfo *sn_info,
> > +                         Error **errp)
> >   {
> >       BlockDriver *drv = bs->drv;
> > -    if (!drv)
> > +
> > +    if (!drv) {
> > +        error_setg(errp, "Device '%s' has no medium.",
>
> In general, error_setg() should not print a trailing '.' (only two
> offenders to git grep 'error_setg.*\."').  I think we also tend to start
> messages with lower case, although that's not as obvious (49 cases of
> 'error_setg[^"*"[A-Z]' vs. 121 of error_setg[^"]*"[a-z]').

I don't know that there is any recommendation for that. I'll fix that in 
next series.

> > +
> > +    error_setg(errp, "Snapshot is not supported for '%s'.",
>
> And again, and probably throughout your series (although I'll quit
> pointing it out).
>
> > @@ -830,16 +832,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
> >        */
> >       if (sn_info->id_str[0] != '\0' &&
> >           strcmp(sn_info->id_str, sn_info->name) != 0) {
> > +        error_setg(errp, "ID and name have to be equal.");
> >           return -EINVAL;
> >       }
> >
> >       if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> > +        error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
>
> s/that/than/
>
> >           return -ERANGE;
> >       }
> >
> >       r = rbd_snap_create(s->image, sn_info->name);
> >       if (r < 0) {
> > -        error_report("failed to create snap: %s", strerror(-r));
> > +        error_setg(errp, "Failed to create snapshot: '%s'.", strerror(-r));
>
> Use error_setg_errno(errp, -r, "failed to create snapshot").
>
>
> > @@ -1779,9 +1781,8 @@ static int sd_snapshot_create(BlockDriverState *bs, 
> > QEMUSnapshotInfo *sn_info)
> >               s->name, sn_info->vm_state_size, s->is_snapshot);
> >
> >       if (s->is_snapshot) {
> > -        error_report("You can't create a snapshot of a snapshot VDI, "
> > -                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
> > -
> > +        error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
> > +                   sn_info->name);
>
> Why are you losing information from the previous version of this error
> message?
>
>
> >       if (ret < 0) {
> > -        error_report("failed to create inode for snapshot. %s",
> > -                     strerror(errno));
> > +        error_setg(errp, "Failed to create inode for snapshot.");
>
> Another case of losing information; error_setg_errno would be better
> here, and anywhere else the old message included a strerror() call.

Thanks for pointing this out. I've probably missed that during the 
rebase. I'll also check all other patches before I'll send another series.

Pavel