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.
signature.asc
Description: PGP signature
