On Fri, 10/21 23:04, Max Reitz wrote: > > +ImageLockMode bdrv_lock_mode_from_flags(int flags) > > +{ > > + if (flags & BDRV_O_NO_LOCK) { > > + return IMAGE_LOCK_MODE_NOLOCK; > > + } else if (flags & BDRV_O_SHARED_LOCK) { > > + return IMAGE_LOCK_MODE_SHARED; > > + } else if (flags & BDRV_O_EXCLUSIVE_LOCK) { > > + return IMAGE_LOCK_MODE_EXCLUSIVE; > > + } else { > > + return IMAGE_LOCK_MODE_AUTO; > > + } > > +} > > I don't know if there's been any discussion about the order of the flags > here, but I personally would order them exactly the other way around: > Asking for exclusive locking should override nolock, in my opinion.
The idea was to assert no two bits are set at the same time. But I seem to have forgotten to actually add the assertion. > > > + > > +ImageLockMode bdrv_get_lock_mode(BlockDriverState *bs) > > +{ > > + return bs->cur_lock; > > +} > > + > > +int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode) > > +{ > > + int ret; > > + > > + if (bs->cur_lock == mode) { > > + return 0; > > + } else if (!bs->drv) { > > + return -ENOMEDIUM; > > + } else if (!bs->drv->bdrv_lockf) { > > + if (bs->file) { > > + return bdrv_set_lock_mode(bs->file->bs, mode); > > + } > > + return 0; > > + } > > + ret = bs->drv->bdrv_lockf(bs, mode); > > + if (ret == -ENOTSUP) { > > + /* Handle it the same way as !bs->drv->bdrv_lockf */ > > + ret = 0; > > Yes, well, why do you handle both as success? Wouldn't returning > -ENOTSUP make more sense? > > I guess the caller can find out itself by checking whether bs->cur_lock > has changed, but... I can't think of a reason for any caller to do something different for -ENOTSUP from success, hence the check here. > > > + } else if (ret == 0) { > > + bs->cur_lock = mode; > > + } > > + return ret; > > +} > > + > > static QemuOptsList bdrv_runtime_opts = { > > .name = "bdrv_common", > > .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head), > > @@ -1076,6 +1119,10 @@ static int bdrv_open_common(BlockDriverState *bs, > > BdrvChild *file, > > goto free_and_fail; > > } > > > > + if (open_flags & BDRV_O_INACTIVE) { > > + open_flags = (open_flags & ~BDRV_O_LOCK_MASK) & BDRV_O_NO_LOCK; > > I suppose the second & is supposed to be a |? Yes. Thanks for catching it. > > > + } > > + > > ret = refresh_total_sectors(bs, bs->total_sectors); > > if (ret < 0) { > > error_setg_errno(errp, -ret, "Could not refresh total sector > > count"); > > @@ -2273,6 +2320,7 @@ static void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > > BdrvChild *child, *next; > > > > + bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK); > > bs->drv->bdrv_close(bs); > > bs->drv = NULL; > > > > @@ -3188,6 +3236,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, > > Error **errp) > > This function's name is pretty weird... Maybe it would be better to > rename it to "bdrv_complete_incoming" or something. (Unrelated to this > series, of course.) > > > error_setg_errno(errp, -ret, "Could not refresh total sector > > count"); > > return; > > } > > + if (bs->cur_lock != IMAGE_LOCK_MODE__MAX) { > > + bdrv_set_lock_mode(bs, bs->cur_lock); > > + } > > } > > > > void bdrv_invalidate_cache_all(Error **errp) > > @@ -3230,6 +3281,7 @@ static int bdrv_inactivate_recurse(BlockDriverState > > *bs, > > } > > > > if (setting_flag) { > > + ret = bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK); > > Maybe it would make sense to do something with the return value...? :-) Yes, sounds good. Fam