From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UsSua-0000G0-Jc for qemu-devel@nongnu.org; Fri, 28 Jun 2013 03:16:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UsSuZ-0002Vi-1x for qemu-devel@nongnu.org; Fri, 28 Jun 2013 03:16:04 -0400 From: Markus Armbruster References: <1372342930-28684-1-git-send-email-armbru@redhat.com> <1372342930-28684-3-git-send-email-armbru@redhat.com> <51CD0476.20301@linux.vnet.ibm.com> Date: Fri, 28 Jun 2013 09:15:57 +0200 In-Reply-To: <51CD0476.20301@linux.vnet.ibm.com> (Lei Li's message of "Fri, 28 Jun 2013 11:35:18 +0800") Message-ID: <874ncipuxe.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/4] Revert "chardev: Make the name of memory device consistent" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lei Li Cc: aliguori@us.ibm.com, qemu-devel , qemu-stable@nongnu.org, lcapitulino@redhat.com, kraxel@redhat.com, Paolo Bonzini Lei Li writes: > On 06/27/2013 10:22 PM, Markus Armbruster wrote: >> This reverts commit 6a85e60cb994bd95d1537aafbff65816f3de4637. >> >> Commit 51767e7 "qemu-char: Add new char backend CirMemCharDriver" >> introduced a memory ring buffer character device driver named >> "memory". Commit 3949e59 "qemu-char: Saner naming of memchar stuff & >> doc fixes" changed the driver name to "ringbuf", along with a whole >> bunch of other names, with the following rationale: >> >> Naming is a mess. The code calls the device driver >> CirMemCharDriver, the public API calls it "memory", "memchardev", >> or "memchar", and the special commands are named like >> "memchar-FOO". "memory" is a particularly unfortunate choice, >> because there's another character device driver called >> MemoryDriver. Moreover, the device's distinctive property is that >> it's a ring buffer, not that's in memory. >> >> This is what we released in 1.4.0. >> >> Unfortunately, the rename missed a critical instance of "memory": the >> actual driver name. Thus, the new device could be used only by an >> entirely undocumented name. The documented name did not work. >> Bummer. > > Hi Markus. > > Actually I fixed this issue as the your coming patch set 3&4 (make them > consistent with 'ringbuf') in the beginning, but according to Paolo's > comments, since '-chardev memory' exists in 1.4 and cannot be renamed, > so made such change as Commit 6a85e60cb994bd95d1537aafbff65816f3de4637. > > The original patch and discussion as link below: > > http://patchwork.ozlabs.org/patch/244848/ Yes, your patch fixes a real bug. However, it changes more than that, and thus results in more "memory" vs. "ringbuf" inconsistency than necessary, as my commit message explains in the part quoted at the end of this message. Some inconsistency is unavoidable unless we forgo backward compatibility to (undocumented!) -chardev memory. We can't get rid of "memory", because -chardev memory "cannot be renamed" (Paolo). I'm not so sure, as it's undocumented, but let's assume we can't. This is one of only two occurences of "memory" before your patch. The other one is ChardevBackend member "memory", which is similar: in 1.4, but undocumented. We can't get rid of "ringbuf", because we can't rename QMP commands ringbuf-read and ringbuf-write. Your patch renames some instances of ringbuf to memory, and leaves others alone. That's why I called the result "an inconsistent mess" in my commit message. My patches rename nothing. They *add* "ringbuf" aliases to the two unfortunate instances of "memory". Both yours and mine fix the "code doesn't match documentation" bug. My patches maximize consistency within the compatibility constraints. I reverted your patch simply because I want to start over for easier review. I should have raised my concerns while your patch was under review. I didn't, because I missed it entirely. My fault, not yours; you cc'ed me. >> Commit 6a85e60 fixes this by changing the documentation to match the >> code. It also changes some, but not all related occurences of >> "ringbuf" to "memory". Left alone are identifiers in C code, HMP and >> QMP commands. The latter are external interface, so they can't be >> changed. >> >> The result is an inconsistent mess. Moreover, "memory" is a rotten >> name. The device's distinctive property is that it's a ring buffer, >> not that's in memory. User's don't care whether it's in RAM, flash, >> or carved into chocolate tablets by Oompa Loompas. >> >> Revert the commit. Next commit will fix just the bug. [...]