On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:
On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
+int bdrv_all_find_snapshot(const char *name, bool read_only,
+ BlockDriverState **first_bad_bs)
+{
+ QEMUSnapshotInfo sn;
+ int err = 0;
+ BlockDriverState *bs = NULL;
+
+ while (err == 0 && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
+ err = bdrv_snapshot_find(bs, &sn, name);
+ }
+ aio_context_release(ctx);
+ }
+
+ *first_bad_bs = bs;
+ return err;
+}
It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:
@@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
available_snapshots = g_new0(int, nb_sns);
total = 0;
for (i = 0; i < nb_sns; i++) {
- sn = &sn_tab[i];
- available = 1;
- bs1 = NULL;
-
- while ((bs1 = bdrv_next(bs1))) {
- if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
- if (ret < 0) {
- available = 0;
- break;
- }
- }
- }
-
- if (available) {
+ if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) {
The original loop skips drives that cannot snapshot and the vmstate
drive. The new code tries to find a snapshot all devices.
To me it seems the semantics are changed. Can you explain why this
change is safe?
argh... This code is used for 'virsh qemu-monitor-command --hmp rhel7
info snapshots'
and NOT used for 'virsh snapshot-list rhel7' or used in a very different
way.
This is the root of the problem.
You are right, the command is broken completely. Some refactoring is
necessary
here.
Den
