From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38025) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vd9Hm-00084W-3t for qemu-devel@nongnu.org; Sun, 03 Nov 2013 20:49:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vd9Hd-0000XN-5U for qemu-devel@nongnu.org; Sun, 03 Nov 2013 20:48:58 -0500 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:40104) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vd9Hc-0000XB-G3 for qemu-devel@nongnu.org; Sun, 03 Nov 2013 20:48:49 -0500 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 4 Nov 2013 11:48:45 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id D3F182CE804A for ; Mon, 4 Nov 2013 12:48:42 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rA41V6H05374352 for ; Mon, 4 Nov 2013 12:31:06 +1100 Received: from d23av03.au.ibm.com (localhost [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rA41mggV006379 for ; Mon, 4 Nov 2013 12:48:42 +1100 Message-ID: <5276FCFC.3080600@linux.vnet.ibm.com> Date: Mon, 04 Nov 2013 09:48:44 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1381787553-12497-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1381787553-12497-3-git-send-email-xiawenc@linux.vnet.ibm.com> <5274F575.6020106@redhat.com> In-Reply-To: <5274F575.6020106@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 2/6] qcow2: add error message in qcow2_write_snapshots() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@gmail.com > On 14.10.2013 23:52, Wenchao Xia wrote: >> The function still returns int since qcow2_snapshot_delete() will >> return the number. >> >> Signed-off-by: Wenchao Xia >> --- >> block/qcow2-snapshot.c | 48 ++++++++++++++++++++++++++++++++++++++++++------ >> 1 files changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index b373f9a..4bd494b 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -152,7 +152,7 @@ fail: >> } >> >> /* add at the end of the file a new list of snapshots */ >> -static int qcow2_write_snapshots(BlockDriverState *bs) >> +static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp) >> { >> BDRVQcowState *s = bs->opaque; >> QCowSnapshot *sn; >> @@ -183,10 +183,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> offset = snapshots_offset; >> if (offset < 0) { >> ret = offset; >> + error_setg(errp, >> + "Failed in allocation of cluster for snapshot list: " >> + "%d (%s)", >> + ret, strerror(-ret)); > > Again, error_setg_errno() is your friend. > >> goto fail; >> } >> ret = bdrv_flush(bs); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in flush after snapshot list cluster allocation: " >> + "%d (%s)", >> + ret, strerror(-ret)); >> goto fail; >> } >> >> @@ -194,6 +202,10 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> * must indeed be completely free */ >> ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in overlap check for snapshot list cluster at %" >> + PRIi64 " with size %d: %d (%s)", >> + offset, snapshots_size, ret, strerror(-ret)); >> goto fail; >> } >> >> @@ -227,24 +239,40 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> >> ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h)); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in write of snapshot header at %" >> + PRIi64 " with size %" PRIu64 ": %d (%s)", >> + offset, sizeof(h), ret, strerror(-ret)); > > Again, on 32 bit systems, sizeof(size_t) is generally 4 and not 8 (you > may want to cast sizeof(h) to int and just use %d). > Maybe caset as (int64_t)sizeof(h) to avoid possible incomplete value? >> goto fail; >> } >> offset += sizeof(h); >> >> ret = bdrv_pwrite(bs->file, offset, &extra, sizeof(extra)); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in write of extra snapshot data at %" >> + PRIi64 " with size %" PRIu64 ": %d (%s)", >> + offset, sizeof(extra), ret, strerror(-ret)); > > Same here. > >> goto fail; >> } >> offset += sizeof(extra); >> >> ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in write of snapshot id string at %" >> + PRIi64 " with size %d: %d (%s)", >> + offset, id_str_size, ret, strerror(-ret)); >> goto fail; >> } >> offset += id_str_size; >> >> ret = bdrv_pwrite(bs->file, offset, sn->name, name_size); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in write of snapshot name string at %" >> + PRIi64 " with size %d: %d (%s)", >> + offset, name_size, ret, strerror(-ret)); >> goto fail; >> } >> offset += name_size; >> @@ -256,6 +284,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> */ >> ret = bdrv_flush(bs); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in flush after snapshot list update: %d (%s)", >> + ret, strerror(-ret)); >> goto fail; >> } >> >> @@ -268,6 +299,11 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots), >> &header_data, sizeof(header_data)); >> if (ret < 0) { >> + error_setg(errp, >> + "Failed in update of image header at %" >> + PRIi64 " with size %" PRIu64 ":%d (%s)", >> + offsetof(QCowHeader, nb_snapshots), sizeof(header_data), >> + ret, strerror(-ret)); > > And here (also applies to offsetof(), which returns size_t as well). > Also, you forgot a space after the colon (although you should be using > error_setg_errno() anyway). > Will use error_setg_errno(). > Max > >> goto fail; >> } >> >> @@ -283,6 +319,9 @@ fail: >> qcow2_free_clusters(bs, snapshots_offset, snapshots_size, >> QCOW2_DISCARD_ALWAYS); >> } >> + if (errp) { >> + g_assert(error_is_set(errp)); >> + } >> return ret; >> } >> >> @@ -446,10 +485,8 @@ void qcow2_snapshot_create(BlockDriverState *bs, >> s->snapshots = new_snapshot_list; >> s->snapshots[s->nb_snapshots++] = *sn; >> >> - ret = qcow2_write_snapshots(bs); >> + ret = qcow2_write_snapshots(bs, errp); >> if (ret < 0) { >> - /* Following line will be replaced with more detailed error later */ >> - error_setg(errp, "Failed in write of snapshot"); >> g_free(s->snapshots); >> s->snapshots = old_snapshot_list; >> s->nb_snapshots--; >> @@ -623,9 +660,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs, >> s->snapshots + snapshot_index + 1, >> (s->nb_snapshots - snapshot_index - 1) * sizeof(sn)); >> s->nb_snapshots--; >> - ret = qcow2_write_snapshots(bs); >> + ret = qcow2_write_snapshots(bs, errp); >> if (ret < 0) { >> - error_setg(errp, "Failed to remove snapshot from snapshot list"); >> return ret; >> } >> > >