Kevin Wolf <kw...@redhat.com> writes:
> Am 12.01.2016 um 16:20 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> writes:
>>
>> > Am 11.01.2016 um 16:49 hat Markus Armbruster geschrieben:
>> >> Eric Blake <ebl...@redhat.com> writes:
>> >>
>> >> > On 12/22/2015 09:46 AM, Kevin Wolf wrote:
>> >> >> This patch extends qemu-img for working with locked images. It prints a
>> >> >> helpful error message when trying to access a locked image read-write,
>> >> >> and adds a 'qemu-img force-unlock' command as well as a 'qemu-img check
>> >> >> -r all --force' option in order to override a lock left behind after a
>> >> >> qemu crash.
>> >> >>
>> >> >> Signed-off-by: Kevin Wolf <kw...@redhat.com>
>> >> >> ---
>> >> >> include/block/block.h | 1 +
>> >> >> include/qapi/error.h | 1 +
>> >> >> qapi/common.json | 3 +-
>> >> >> qemu-img-cmds.hx | 10 ++++--
>> >> >> qemu-img.c | 96
>> >> >> +++++++++++++++++++++++++++++++++++++++++++--------
>> >> >> qemu-img.texi | 20 ++++++++++-
>> >> >> 6 files changed, 113 insertions(+), 18 deletions(-)
>> >> >>
>> >> >
>> >> >> +++ b/include/qapi/error.h
>> >> >> @@ -102,6 +102,7 @@ typedef enum ErrorClass {
>> >> >> ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICENOTACTIVE,
>> >> >> ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICENOTFOUND,
>> >> >> ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVMMISSINGCAP,
>> >> >> + ERROR_CLASS_IMAGE_FILE_LOCKED = QAPI_ERROR_CLASS_IMAGEFILELOCKED,
>> >> >> } ErrorClass;
>> >> >
>> >> > Wow - a new ErrorClass. It's been a while since we could justify one of
>> >> > these, but I think you might have found a case.
>> >>
>> >> Spell out the rationale for the new ErrorClass, please.
>> >
>> > Action to be taken for this error class: Decide whether the lock is a
>> > leftover from a previous qemu run that ended in an unclean shutdown. If
>> > so, retry with overriding the lock.
>> >
>> > Currently used by qemu-img when ordered to override a lock. libvirt
>> > will need to do the same.
>>
>> Let's see whether I understand the intended use:
>>
>> open image
>> if open fails with ImageFileLocked:
>> guess whether the lock is stale
>> if guessing not stale:
>> error out
>> open image with lock override
>>
>> Correct?
>
> Yes. Where "guess" is more or less "check whether the management tool
> started qemu with this image, but didn't cleanly shut it down". This can
> guess wrong if, and only if, some other user used a different algorithm
> and forced an unlock even though the image didn't belong to them before
> the crash.
>
>> Obvious troublespots:
>>
>> 1. If you guess wrong, you destroy the image. No worse than before, so
>> okay, declare documentation problem.
>>
>> 2. TOCTTOU open to open with lock override
>> [...]
>>
>> 3. TOCTTOU within open (hypothetical, haven't read your code)
>> [...]
>
> Yes, these exist in theory. The question is what scenarios you want to
> protect against and whether improving the mechanism to cover these cases
> is worth the effort.
>
> The answer for what I wanted to protect is a manual action on an image
> that is already in use. The user isn't quick enough to manually let two
> processes open the same image at the same time, so I didn't consider
> that scenario relevant.
>
> But assuming that everyone (including the human user) follows the above
> protocol (force-unlock only what was yours before the crash), at least
> cases 1 and 2 don't happen anyway.
"Force-unlock only what you locked yourself" is easier to stipulate than
to adhere to when the tools can't give you a hint on who did the
locking. This is particularly true when "you" is a human, with human
imperfect memory.
I understand that this locking can't provide complete protection, and
merely aims to catch certain common accidents.
However, to avoid a false sense of security, its limitations need to be
clearly documented. This very much includes the rule "force-unlock only
what you locked yourself". In my opinion, it should also include the
raciness.
Sometimes, solving a problem is easier than documenting it.
>> Let me try a different tack. It may well be unworkable.
>> [...]
>
> It doesn't sound unworkable, but it might be overengineered if the goal
> is just to protect people against 'qemu-img snapshot' on running VMs.
>
>> Obviously, the management application will also need to be able to
>> override stale locks from opens by someone else, say a human user
>> bypassing the management application.
>
> That's not obvious. If another user messed it up, this other user can
> also clean it up. But yes, asking a higher level would work, too.
>
> Kevin
