| Message ID | 1361875228-15769-6-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > This patch add function bdrv_query_snapshot_infolist(), which will adds > return snapshot info of an image in qmp object format. The implementation > code are based on the code moved from qemu-img.c with modification to fit more implementation is based > for qmp based block layer API. > To help filter out snapshot info not needed, a call back function is > added in bdrv_query_snapshot_infolist(). > bdrv_can_read_snapshot() should be called before call this function, > to avoid got *errp set unexpectedly. "avoid getting" Not sure about "should". Couldn't a caller safely call this with a non-snapshot bs argument, and simply deal with the error? Long lines, and unusual paragraph formatting. Here's how we usually do this: This patch adds function bdrv_query_snapshot_infolist(), which will return snapshot info of an image in qmp object format. The implementation is based on the code moved from qemu-img.c with modification to fit more for qmp based block layer API. To help filter out snapshot info not needed, a call back function is added in bdrv_query_snapshot_infolist(). bdrv_can_read_snapshot() should be called before call this function, to avoid getting *errp set unexpectedly. > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block.c | 60 ++++++++++++++++++++++++++++++++++++++---------- > include/block/block.h | 8 +++++- > qemu-img.c | 8 +++++- > 3 files changed, 61 insertions(+), 15 deletions(-) > > diff --git a/block.c b/block.c > index 368b37c..8d0145a 100644 > --- a/block.c > +++ b/block.c > @@ -2813,29 +2813,62 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, > return 0; > } > > -void collect_snapshots(BlockDriverState *bs , ImageInfo *info) > +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, > + SnapshotFilterFunc filter, > + void *opaque, > + Error **errp) > { > int i, sn_count; > QEMUSnapshotInfo *sn_tab = NULL; > - SnapshotInfoList *info_list, *cur_item = NULL; > + SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL; > + SnapshotInfo *info; > + > sn_count = bdrv_snapshot_list(bs, &sn_tab); > + if (sn_count < 0) { > + /* Now bdrv_snapshot_list() use negative value to tip error, so a check > + * of it was done here. In future errp can be set by that function > + * itself, by changing the call back functions in c files in ./block. > + */ I doubt we need this comment. > + const char *dev = bdrv_get_device_name(bs); > + switch (sn_count) { > + case -ENOMEDIUM: > + error_setg(errp, "Device '%s' is not inserted.", dev); We generally don't end error messages with a period. Please remove it from this and other messages. > + break; > + case -ENOTSUP: > + error_setg(errp, > + "Device '%s' does not support internal snapshot.", internal snapshots > + dev); > + break; > + default: > + error_setg(errp, > + "Device '%s' got '%d' for bdrv_snapshot_list(), " > + "message '%s'.", > + dev, sn_count, strerror(-sn_count)); error_setg(errp, "Can't list snapshots of device '%s': %s", dev, sterror(-sn_count)); > + break; > + } > + return NULL; > + } > > for (i = 0; i < sn_count; i++) { > - info->has_snapshots = true; > - info_list = g_new0(SnapshotInfoList, 1); > + if (filter && filter(&sn_tab[i], opaque) != 0) { > + continue; > + } Is the filter feature really worth it? If yes, should it be added in a separate patch? > > - info_list->value = g_new0(SnapshotInfo, 1); > - info_list->value->id = g_strdup(sn_tab[i].id_str); > - info_list->value->name = g_strdup(sn_tab[i].name); > - info_list->value->vm_state_size = sn_tab[i].vm_state_size; > - info_list->value->date_sec = sn_tab[i].date_sec; > - info_list->value->date_nsec = sn_tab[i].date_nsec; > - info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; > - info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; > + info = g_new0(SnapshotInfo, 1); > + info->id = g_strdup(sn_tab[i].id_str); > + info->name = g_strdup(sn_tab[i].name); > + info->vm_state_size = sn_tab[i].vm_state_size; > + info->date_sec = sn_tab[i].date_sec; > + info->date_nsec = sn_tab[i].date_nsec; > + info->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; > + info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; > + > + info_list = g_new0(SnapshotInfoList, 1); > + info_list->value = info; > > /* XXX: waiting for the qapi to support qemu-queue.h types */ > if (!cur_item) { > - info->snapshots = cur_item = info_list; > + head = cur_item = info_list; > } else { > cur_item->next = info_list; > cur_item = info_list; > @@ -2844,6 +2877,7 @@ void collect_snapshots(BlockDriverState *bs , ImageInfo *info) > } > > g_free(sn_tab); > + return head; > } > > void collect_image_info(BlockDriverState *bs, > diff --git a/include/block/block.h b/include/block/block.h > index e6d915c..51a14f2 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -319,6 +319,13 @@ void bdrv_get_backing_filename(BlockDriverState *bs, > char *filename, int filename_size); > void bdrv_get_full_backing_filename(BlockDriverState *bs, > char *dest, size_t sz); > + > +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque); > +/* assume bs is already opened, use qapi_free_* to free returned value. */ Pretty much all functions assume bs is open, hardly worth a comment. > +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, > + SnapshotFilterFunc filter, > + void *opaque, > + Error **errp); > BlockInfo *bdrv_query_info(BlockDriverState *s); > BlockStats *bdrv_query_stats(const BlockDriverState *bs); > bool bdrv_can_read_snapshot(BlockDriverState *bs); > @@ -450,7 +457,6 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event, > int bdrv_debug_resume(BlockDriverState *bs, const char *tag); > bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag); > > -void collect_snapshots(BlockDriverState *bs , ImageInfo *info); > void collect_image_info(BlockDriverState *bs, > ImageInfo *info, > const char *filename); > diff --git a/qemu-img.c b/qemu-img.c > index b650d17..1034cc5 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1280,7 +1280,13 @@ static ImageInfoList *collect_image_info_list(const char *filename, > > info = g_new0(ImageInfo, 1); > collect_image_info(bs, info, filename); > - collect_snapshots(bs, info); > + if (bdrv_can_read_snapshot(bs)) { > + info->snapshots = bdrv_query_snapshot_infolist(bs, NULL, > + NULL, NULL); > + if (info->snapshots) { > + info->has_snapshots = true; > + } > + } > > elem = g_new0(ImageInfoList, 1); > elem->value = info; Silently ignores errors, but the old code does so, too. QAPI's has_FOO is pointless when FOO is a pointer. Not your fault.
于 2013-2-27 22:26, Markus Armbruster 写道: > Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > >> This patch add function bdrv_query_snapshot_infolist(), which will > > adds > >> return snapshot info of an image in qmp object format. The implementation >> code are based on the code moved from qemu-img.c with modification to fit more > > implementation is based > >> for qmp based block layer API. >> To help filter out snapshot info not needed, a call back function is >> added in bdrv_query_snapshot_infolist(). >> bdrv_can_read_snapshot() should be called before call this function, >> to avoid got *errp set unexpectedly. > > "avoid getting" > > Not sure about "should". Couldn't a caller safely call this with a > non-snapshot bs argument, and simply deal with the error? > yes, but in some case this error should be avoided. For eg, in bdrv_query_image_info(), if a image is not inserted, or it can't take snapshot, other info should also be returned without error, so if bdrv_can_read_snapshot() is called before, this kind of error is avoided, and caller can check if there is other error. I guess it should be documented as "in some case, bdrv_can_read_snapshot() should be called." > Long lines, and unusual paragraph formatting. Here's how we usually do > this: > > This patch adds function bdrv_query_snapshot_infolist(), which will > return snapshot info of an image in qmp object format. The > implementation is based on the code moved from qemu-img.c with > modification to fit more for qmp based block layer API. > > To help filter out snapshot info not needed, a call back function is > added in bdrv_query_snapshot_infolist(). > > bdrv_can_read_snapshot() should be called before call this function, to > avoid getting *errp set unexpectedly. > Thanks, going to use this. >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> block.c | 60 ++++++++++++++++++++++++++++++++++++++---------- >> include/block/block.h | 8 +++++- >> qemu-img.c | 8 +++++- >> 3 files changed, 61 insertions(+), 15 deletions(-) >> >> diff --git a/block.c b/block.c >> index 368b37c..8d0145a 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2813,29 +2813,62 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, >> return 0; >> } >> >> -void collect_snapshots(BlockDriverState *bs , ImageInfo *info) >> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, >> + SnapshotFilterFunc filter, >> + void *opaque, >> + Error **errp) >> { >> int i, sn_count; >> QEMUSnapshotInfo *sn_tab = NULL; >> - SnapshotInfoList *info_list, *cur_item = NULL; >> + SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL; >> + SnapshotInfo *info; >> + >> sn_count = bdrv_snapshot_list(bs, &sn_tab); >> + if (sn_count < 0) { >> + /* Now bdrv_snapshot_list() use negative value to tip error, so a check >> + * of it was done here. In future errp can be set by that function >> + * itself, by changing the call back functions in c files in ./block. >> + */ > > I doubt we need this comment. > I am OK to remove it. >> + const char *dev = bdrv_get_device_name(bs); >> + switch (sn_count) { >> + case -ENOMEDIUM: >> + error_setg(errp, "Device '%s' is not inserted.", dev); > > We generally don't end error messages with a period. Please remove it > from this and other messages. > OK. >> + break; >> + case -ENOTSUP: >> + error_setg(errp, >> + "Device '%s' does not support internal snapshot.", > > internal snapshots > OK. >> + dev); >> + break; >> + default: >> + error_setg(errp, >> + "Device '%s' got '%d' for bdrv_snapshot_list(), " >> + "message '%s'.", >> + dev, sn_count, strerror(-sn_count)); > > error_setg(errp, "Can't list snapshots of device '%s': %s", > dev, sterror(-sn_count)); > OK. >> + break; >> + } >> + return NULL; >> + } >> >> for (i = 0; i < sn_count; i++) { >> - info->has_snapshots = true; >> - info_list = g_new0(SnapshotInfoList, 1); >> + if (filter && filter(&sn_tab[i], opaque) != 0) { >> + continue; >> + } > > Is the filter feature really worth it? If yes, should it be added in a > separate patch? > I feel filter make logic more clearer later which can be used later to filter out inconsistent snapshots. I am OK to form a seperate patch. >> >> - info_list->value = g_new0(SnapshotInfo, 1); >> - info_list->value->id = g_strdup(sn_tab[i].id_str); >> - info_list->value->name = g_strdup(sn_tab[i].name); >> - info_list->value->vm_state_size = sn_tab[i].vm_state_size; >> - info_list->value->date_sec = sn_tab[i].date_sec; >> - info_list->value->date_nsec = sn_tab[i].date_nsec; >> - info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; >> - info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; >> + info = g_new0(SnapshotInfo, 1); >> + info->id = g_strdup(sn_tab[i].id_str); >> + info->name = g_strdup(sn_tab[i].name); >> + info->vm_state_size = sn_tab[i].vm_state_size; >> + info->date_sec = sn_tab[i].date_sec; >> + info->date_nsec = sn_tab[i].date_nsec; >> + info->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; >> + info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; >> + >> + info_list = g_new0(SnapshotInfoList, 1); >> + info_list->value = info; >> >> /* XXX: waiting for the qapi to support qemu-queue.h types */ >> if (!cur_item) { >> - info->snapshots = cur_item = info_list; >> + head = cur_item = info_list; >> } else { >> cur_item->next = info_list; >> cur_item = info_list; >> @@ -2844,6 +2877,7 @@ void collect_snapshots(BlockDriverState *bs , ImageInfo *info) >> } >> >> g_free(sn_tab); >> + return head; >> } >> >> void collect_image_info(BlockDriverState *bs, >> diff --git a/include/block/block.h b/include/block/block.h >> index e6d915c..51a14f2 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -319,6 +319,13 @@ void bdrv_get_backing_filename(BlockDriverState *bs, >> char *filename, int filename_size); >> void bdrv_get_full_backing_filename(BlockDriverState *bs, >> char *dest, size_t sz); >> + >> +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque); >> +/* assume bs is already opened, use qapi_free_* to free returned value. */ > > Pretty much all functions assume bs is open, hardly worth a comment. > OK, will remove. >> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, >> + SnapshotFilterFunc filter, >> + void *opaque, >> + Error **errp); >> BlockInfo *bdrv_query_info(BlockDriverState *s); >> BlockStats *bdrv_query_stats(const BlockDriverState *bs); >> bool bdrv_can_read_snapshot(BlockDriverState *bs); >> @@ -450,7 +457,6 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event, >> int bdrv_debug_resume(BlockDriverState *bs, const char *tag); >> bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag); >> >> -void collect_snapshots(BlockDriverState *bs , ImageInfo *info); >> void collect_image_info(BlockDriverState *bs, >> ImageInfo *info, >> const char *filename); >> diff --git a/qemu-img.c b/qemu-img.c >> index b650d17..1034cc5 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1280,7 +1280,13 @@ static ImageInfoList *collect_image_info_list(const char *filename, >> >> info = g_new0(ImageInfo, 1); >> collect_image_info(bs, info, filename); >> - collect_snapshots(bs, info); >> + if (bdrv_can_read_snapshot(bs)) { >> + info->snapshots = bdrv_query_snapshot_infolist(bs, NULL, >> + NULL, NULL); >> + if (info->snapshots) { >> + info->has_snapshots = true; >> + } >> + } >> >> elem = g_new0(ImageInfoList, 1); >> elem->value = info; > > Silently ignores errors, but the old code does so, too. > OK, error will be checked. > QAPI's has_FOO is pointless when FOO is a pointer. Not your fault. >
On Tue, Feb 26, 2013 at 06:40:19PM +0800, Wenchao Xia wrote: > diff --git a/include/block/block.h b/include/block/block.h > index e6d915c..51a14f2 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -319,6 +319,13 @@ void bdrv_get_backing_filename(BlockDriverState *bs, > char *filename, int filename_size); > void bdrv_get_full_backing_filename(BlockDriverState *bs, > char *dest, size_t sz); > + > +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque); > +/* assume bs is already opened, use qapi_free_* to free returned value. */ > +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, > + SnapshotFilterFunc filter, > + void *opaque, > + Error **errp); bdrv_query_snapshot_infolist() uses SnapshotInfoList, therefore it makes sense to pass a SnapshotInfo argument to SnapshotFilterFunc() instead of a QEMUSnapshotInfo. The QEMUSnapshotInfo type is an implementation detail that should not be part of the API. > diff --git a/qemu-img.c b/qemu-img.c > index b650d17..1034cc5 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1280,7 +1280,13 @@ static ImageInfoList *collect_image_info_list(const char *filename, > > info = g_new0(ImageInfo, 1); > collect_image_info(bs, info, filename); > - collect_snapshots(bs, info); > + if (bdrv_can_read_snapshot(bs)) { > + info->snapshots = bdrv_query_snapshot_infolist(bs, NULL, > + NULL, NULL); > + if (info->snapshots) { > + info->has_snapshots = true; > + } > + } bdrv_can_read_snapshot() is not needed. The code works fine without the if check.
On Thu, Feb 28, 2013 at 09:53:53AM +0800, Wenchao Xia wrote: > 于 2013-2-27 22:26, Markus Armbruster 写道: > > Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > > > >> This patch add function bdrv_query_snapshot_infolist(), which will > > > > adds > > > >> return snapshot info of an image in qmp object format. The implementation > >> code are based on the code moved from qemu-img.c with modification to fit more > > > > implementation is based > > > >> for qmp based block layer API. > >> To help filter out snapshot info not needed, a call back function is > >> added in bdrv_query_snapshot_infolist(). > >> bdrv_can_read_snapshot() should be called before call this function, > >> to avoid got *errp set unexpectedly. > > > > "avoid getting" > > > > Not sure about "should". Couldn't a caller safely call this with a > > non-snapshot bs argument, and simply deal with the error? > > > yes, but in some case this error should be avoided. For eg, in > bdrv_query_image_info(), if a image is not inserted, or it can't > take snapshot, other info should also be returned without error, > so if bdrv_can_read_snapshot() is called before, this kind of > error is avoided, and caller can check if there is other > error. I guess it should be documented as "in some case, > bdrv_can_read_snapshot() should be called." Is the problem that we cannot test what an Error instance means? So the caller cannot test the Error to find out whether the medium is not inserted. It's cleaner to keep a -errno return value *plus* have an Error* argument. That way QEMU callers can distinguish the error meaning and we don't have to create a separate function that gets called first to test very specific conditions that aren't meaningful by themselves. Stefan
于 2013-2-28 23:41, Stefan Hajnoczi 写道: > On Thu, Feb 28, 2013 at 09:53:53AM +0800, Wenchao Xia wrote: >> 于 2013-2-27 22:26, Markus Armbruster 写道: >>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: >>> >>>> This patch add function bdrv_query_snapshot_infolist(), which will >>> >>> adds >>> >>>> return snapshot info of an image in qmp object format. The implementation >>>> code are based on the code moved from qemu-img.c with modification to fit more >>> >>> implementation is based >>> >>>> for qmp based block layer API. >>>> To help filter out snapshot info not needed, a call back function is >>>> added in bdrv_query_snapshot_infolist(). >>>> bdrv_can_read_snapshot() should be called before call this function, >>>> to avoid got *errp set unexpectedly. >>> >>> "avoid getting" >>> >>> Not sure about "should". Couldn't a caller safely call this with a >>> non-snapshot bs argument, and simply deal with the error? >>> >> yes, but in some case this error should be avoided. For eg, in >> bdrv_query_image_info(), if a image is not inserted, or it can't >> take snapshot, other info should also be returned without error, >> so if bdrv_can_read_snapshot() is called before, this kind of >> error is avoided, and caller can check if there is other >> error. I guess it should be documented as "in some case, >> bdrv_can_read_snapshot() should be called." > > Is the problem that we cannot test what an Error instance means? So the > caller cannot test the Error to find out whether the medium is not > inserted. > > It's cleaner to keep a -errno return value *plus* have an Error* > argument. That way QEMU callers can distinguish the error meaning and > we don't have to create a separate function that gets called first to > test very specific conditions that aren't meaningful by themselves. > > Stefan > This is also a solution, but let me check if the function below can return correct error value now.
于 2013-2-28 23:36, Stefan Hajnoczi 写道: > On Tue, Feb 26, 2013 at 06:40:19PM +0800, Wenchao Xia wrote: >> diff --git a/include/block/block.h b/include/block/block.h >> index e6d915c..51a14f2 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -319,6 +319,13 @@ void bdrv_get_backing_filename(BlockDriverState *bs, >> char *filename, int filename_size); >> void bdrv_get_full_backing_filename(BlockDriverState *bs, >> char *dest, size_t sz); >> + >> +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque); >> +/* assume bs is already opened, use qapi_free_* to free returned value. */ >> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, >> + SnapshotFilterFunc filter, >> + void *opaque, >> + Error **errp); > > bdrv_query_snapshot_infolist() uses SnapshotInfoList, therefore it makes > sense to pass a SnapshotInfo argument to SnapshotFilterFunc() instead of > a QEMUSnapshotInfo. The QEMUSnapshotInfo type is an implementation > detail that should not be part of the API. > OK. >> diff --git a/qemu-img.c b/qemu-img.c >> index b650d17..1034cc5 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1280,7 +1280,13 @@ static ImageInfoList *collect_image_info_list(const char *filename, >> >> info = g_new0(ImageInfo, 1); >> collect_image_info(bs, info, filename); >> - collect_snapshots(bs, info); >> + if (bdrv_can_read_snapshot(bs)) { >> + info->snapshots = bdrv_query_snapshot_infolist(bs, NULL, >> + NULL, NULL); >> + if (info->snapshots) { >> + info->has_snapshots = true; >> + } >> + } > > bdrv_can_read_snapshot() is not needed. The code works fine without the > if check. > OK.
diff --git a/block.c b/block.c index 368b37c..8d0145a 100644 --- a/block.c +++ b/block.c @@ -2813,29 +2813,62 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top, return 0; } -void collect_snapshots(BlockDriverState *bs , ImageInfo *info) +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, + SnapshotFilterFunc filter, + void *opaque, + Error **errp) { int i, sn_count; QEMUSnapshotInfo *sn_tab = NULL; - SnapshotInfoList *info_list, *cur_item = NULL; + SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL; + SnapshotInfo *info; + sn_count = bdrv_snapshot_list(bs, &sn_tab); + if (sn_count < 0) { + /* Now bdrv_snapshot_list() use negative value to tip error, so a check + * of it was done here. In future errp can be set by that function + * itself, by changing the call back functions in c files in ./block. + */ + const char *dev = bdrv_get_device_name(bs); + switch (sn_count) { + case -ENOMEDIUM: + error_setg(errp, "Device '%s' is not inserted.", dev); + break; + case -ENOTSUP: + error_setg(errp, + "Device '%s' does not support internal snapshot.", + dev); + break; + default: + error_setg(errp, + "Device '%s' got '%d' for bdrv_snapshot_list(), " + "message '%s'.", + dev, sn_count, strerror(-sn_count)); + break; + } + return NULL; + } for (i = 0; i < sn_count; i++) { - info->has_snapshots = true; - info_list = g_new0(SnapshotInfoList, 1); + if (filter && filter(&sn_tab[i], opaque) != 0) { + continue; + } - info_list->value = g_new0(SnapshotInfo, 1); - info_list->value->id = g_strdup(sn_tab[i].id_str); - info_list->value->name = g_strdup(sn_tab[i].name); - info_list->value->vm_state_size = sn_tab[i].vm_state_size; - info_list->value->date_sec = sn_tab[i].date_sec; - info_list->value->date_nsec = sn_tab[i].date_nsec; - info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; - info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; + info = g_new0(SnapshotInfo, 1); + info->id = g_strdup(sn_tab[i].id_str); + info->name = g_strdup(sn_tab[i].name); + info->vm_state_size = sn_tab[i].vm_state_size; + info->date_sec = sn_tab[i].date_sec; + info->date_nsec = sn_tab[i].date_nsec; + info->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000; + info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000; + + info_list = g_new0(SnapshotInfoList, 1); + info_list->value = info; /* XXX: waiting for the qapi to support qemu-queue.h types */ if (!cur_item) { - info->snapshots = cur_item = info_list; + head = cur_item = info_list; } else { cur_item->next = info_list; cur_item = info_list; @@ -2844,6 +2877,7 @@ void collect_snapshots(BlockDriverState *bs , ImageInfo *info) } g_free(sn_tab); + return head; } void collect_image_info(BlockDriverState *bs, diff --git a/include/block/block.h b/include/block/block.h index e6d915c..51a14f2 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -319,6 +319,13 @@ void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz); + +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque); +/* assume bs is already opened, use qapi_free_* to free returned value. */ +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs, + SnapshotFilterFunc filter, + void *opaque, + Error **errp); BlockInfo *bdrv_query_info(BlockDriverState *s); BlockStats *bdrv_query_stats(const BlockDriverState *bs); bool bdrv_can_read_snapshot(BlockDriverState *bs); @@ -450,7 +457,6 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event, int bdrv_debug_resume(BlockDriverState *bs, const char *tag); bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag); -void collect_snapshots(BlockDriverState *bs , ImageInfo *info); void collect_image_info(BlockDriverState *bs, ImageInfo *info, const char *filename); diff --git a/qemu-img.c b/qemu-img.c index b650d17..1034cc5 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1280,7 +1280,13 @@ static ImageInfoList *collect_image_info_list(const char *filename, info = g_new0(ImageInfo, 1); collect_image_info(bs, info, filename); - collect_snapshots(bs, info); + if (bdrv_can_read_snapshot(bs)) { + info->snapshots = bdrv_query_snapshot_infolist(bs, NULL, + NULL, NULL); + if (info->snapshots) { + info->has_snapshots = true; + } + } elem = g_new0(ImageInfoList, 1); elem->value = info;