From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47938) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKV12-00088Q-KK for qemu-devel@nongnu.org; Tue, 26 Mar 2013 10:38:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKV11-0004fm-3q for qemu-devel@nongnu.org; Tue, 26 Mar 2013 10:38:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55222) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKV10-0004ec-T5 for qemu-devel@nongnu.org; Tue, 26 Mar 2013 10:38:19 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2QEcHQ3026961 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Mar 2013 10:38:17 -0400 Message-ID: <5151B2D7.9070506@redhat.com> Date: Tue, 26 Mar 2013 15:38:15 +0100 From: Pavel Hrdina MIME-Version: 1.0 References: <5752796f1573f8ccd8de9224a5ce15f71908ac2d.1363957855.git.phrdina@redhat.com> <5151AF3A.6070801@redhat.com> In-Reply-To: <5151AF3A.6070801@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 01/12] block: add error parameter to bdrv_snapshot_create() and related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 26.3.2013 15:22, Eric Blake wrote: > On 03/22/2013 07:16 AM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina >> --- >> 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