| Message ID | 1381787553-12497-3-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <xiawenc@linux.vnet.ibm.com> > --- > 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). > 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). 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; > } >
> 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 <xiawenc@linux.vnet.ibm.com> >> --- >> 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; >> } >> > >
On 11/03/2013 06:48 PM, Wenchao Xia wrote: >> On 14.10.2013 23:52, Wenchao Xia wrote: >>> The function still returns int since qcow2_snapshot_delete() will >>> return the number. >>> >>> @@ -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? If we have objects larger than 2G (such that sizeof(h) would require more than 31 bits), we have other issues to worry about. Casting to int and using %d is safe enough.
δΊ 2013/11/5 3:46, Eric Blake ει: > On 11/03/2013 06:48 PM, Wenchao Xia wrote: >>> On 14.10.2013 23:52, Wenchao Xia wrote: >>>> The function still returns int since qcow2_snapshot_delete() will >>>> return the number. >>>> > > >>>> @@ -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? > > If we have objects larger than 2G (such that sizeof(h) would require > more than 31 bits), we have other issues to worry about. Casting to int > and using %d is safe enough. > OK, will use %d.
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)); 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)); 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)); 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)); 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; }
The function still returns int since qcow2_snapshot_delete() will return the number. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- block/qcow2-snapshot.c | 48 ++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 42 insertions(+), 6 deletions(-)