From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zyaaj-0006ed-Vw for qemu-devel@nongnu.org; Tue, 17 Nov 2015 02:22:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zyaaf-0000wG-1v for qemu-devel@nongnu.org; Tue, 17 Nov 2015 02:22:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zyaae-0000wC-Sk for qemu-devel@nongnu.org; Tue, 17 Nov 2015 02:22:08 -0500 Date: Tue, 17 Nov 2015 15:22:01 +0800 From: Stefan Hajnoczi Message-ID: <20151117072201.GI16268@stefanha-x1.localdomain> References: <1447687481-29244-1-git-send-email-den@openvz.org> <1447687481-29244-6-git-send-email-den@openvz.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7SrMUQONj8Rl9QNG" Content-Disposition: inline In-Reply-To: <1447687481-29244-6-git-send-email-den@openvz.org> Subject: Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Kevin Wolf , qemu-devel@nongnu.org, Juan Quintela --7SrMUQONj8Rl9QNG Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Nov 16, 2015 at 06:24:36PM +0300, Denis V. Lunev wrote: > +int bdrv_all_find_snapshot(const char *name, bool skip_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); > + > + if (skip_read_only && > + (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs))) { These must be called with AioContext acquired. > + continue; > + } > + > + aio_context_acquire(ctx); > + if (bdrv_can_snapshot(bs)) { The !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) checks above are redundant since bdrv_can_snapshot() checks too: int bdrv_can_snapshot(BlockDriverState *bs) { BlockDriver *drv = bs->drv; if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) { return 0; } It also means that the "skip_read_only" name is inaccurate. Read-only drives are always skipped, regardless of skip_read_only's value. The skip_read_only argument can be dropped and the earlier !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) check can be dropped too. > @@ -2168,21 +2157,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, false, &bs1) == 0) { bdrv_all_find_snapshot() doesn't do the bs1 != bs exclusion so the new code behaves differently from the old code. That seems like a bug. --7SrMUQONj8Rl9QNG Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJWStWZAAoJEJykq7OBq3PIypoH/j8W4YB54utE8aShb63aducV x391DqesOYYxZ82D0rEfDTn9BK0DVh4+jpZSWwdPYFXGKxHrWhtrRmocX326HceA ToaSzgqcNKigyLLCoJ/L4DFADBmsiYgue7lVpJh0r1OLSfDBQPWykEQkX2KMq0+r sxo8TFJXEfmpiv4yln3qOxzI6GEm6G16FIZbtKmgS0Df6tbANpZ1FL3AU7RfJxXq It++rNAOOtRdqOVZga6y9Db5TJDg4GCiVdiZmzGsVVm/M+Dfi5BBfVJIJAwFD6RH AKjpzK+I02o5u6Cx5a6kyp4mD+019MOKwxzr+SEXAEXs+S3Iyk9rhVwu3N+eLJY= =R9yo -----END PGP SIGNATURE----- --7SrMUQONj8Rl9QNG--