From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xwstd-0007jt-W7 for qemu-devel@nongnu.org; Fri, 05 Dec 2014 08:26:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XwstX-0006jw-KS for qemu-devel@nongnu.org; Fri, 05 Dec 2014 08:26:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48365) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XwstX-0006jn-79 for qemu-devel@nongnu.org; Fri, 05 Dec 2014 08:26:03 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB5DQ2WB010971 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 5 Dec 2014 08:26:02 -0500 Message-ID: <5481B267.2020706@redhat.com> Date: Fri, 05 Dec 2014 14:25:59 +0100 From: Max Reitz MIME-Version: 1.0 References: <1417774136-30001-1-git-send-email-mreitz@redhat.com> <5481AFA2.2040507@redhat.com> In-Reply-To: <5481AFA2.2040507@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Markus Armbruster , Stefan Hajnoczi , Luiz Capitulino On 2014-12-05 at 14:14, Eric Blake wrote: > On 12/05/2014 03:08 AM, Max Reitz wrote: >> The 'change' QMP and HMP command allows replacing the medium in drives >> which support this, e.g. floppy disk drives. For some drives, the medium >> carries information about whether it can be written to or not (again, >> floppy drives). Therefore, it should be possible to change the read-only >> state of block devices when changing the loaded medium. >> >> Following a suggestion from Eric, this series first introduces a >> 'blockdev-change-medium' QMP command which is intended to replace the >> 'change' command for block devices. Then, an optional additional >> 'read-only' parameter is added which allows chaning the read-only state > s/chaning/changing/ (I first read it as chaining) - of course, typos in > cover letters don't really matter in the long run :) > >> in three ways: >> >> - 'retain': Just keep the status as it was before; this is the current >> behavior and thus this will be the default. >> - 'ro': Force read-only access >> - 'rw': Force writable access >> >> Finally, that 'read-only' parameter is added to the HMP 'change' >> command. This series does not add a 'blockdev-change-medium' QMP command > I assume you meant HMP in this line. Yes, right. >> because 'change' being overloaded for VNC and block devices is not too >> bad for HMP (while it is for QMP). > I agree with that approach. > >> >> v2: >> - basically completely rewritten >> - Dropped 'auto' [Kevin and Markus] >> - Introduced blockdev-change-medium [Eric] >> >> - Patch 1 introduces the new QMP command 'blockdev-change-medium'; there >> are (at least) two questionable design choices which I want to explain >> here: >> - The name is rather long; furthermore, the name 'change-blockdev' was >> already suggested by the existing code. I used such a long name >> because (1) there are no *-blockdev commands, but there are >> blockdev-* commands, so "blockdev" should be the prefix, not the >> suffix, and (2) "blockdev-change" could mean anything, so I wanted >> to be as clear as possible. > That's actually a good explanation; I'm fine with the name you ended up > with, even if it feels long. > >> - The 'format' argument is optional; this is because by making it >> mandatory, it would have been difficult for the 'change' QMP and HMP >> commands to retain their 'format' argument optional as well (which >> we have to do thanks to compatibility) > Yep, I can see that. On the other hand, the other questionable feature > of 'change' was that it required a filename, but then allowed "" as the > filename that meant no new medium. Hm, really? HMP: (qemu) change fdd "" Could not open image: No such file or directory QMP: {'execute':'change','arguments':{'device':'fdd','target':''}} {"timestamp": {"seconds": 1417785755, "microseconds": 519130}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "fdd", "tray-open": true}} {"error": {"class": "GenericError", "desc": "Could not open image: No such file or directory"}} Technically, the medium is now empty, but that wasn't a successful execution. > In patch 1/4, I wonder if we should > make the new command a bit stricter, by having 'filename' be optional, > and by forbidding "" as a filename (it's still a 1:1 mapping to the old > semantics, and while it would require more HMP glue, it would feel a bit > cleaner from the interface side). If it was indeed possible somehow, I'd vote for disallowing empty file names for blockdev-change-medium and instead invoke 'eject' from the 'change' commands (and point to 'eject' in the 'change' documentation). Max