[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of mon

From: Max Reitz
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends
Date: Mon, 22 Feb 2016 17:29:39 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

On 22.02.2016 09:24, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
>> On 17.02.2016 17:20, Kevin Wolf wrote:
>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
>>>>>> The monitor does hold references to some BlockBackends so it should have
>>>>> s/does hold/holds/?
>>>> It was intentional, so I'd keep it unless you drop the question mark.
>>> For me it seems to imply something like "contrary to your expectations",
>>> but maybe that's just my non-native English needing a fix.
>>> I don't find it surprising anyway that the monitor holds BB references.
> Me neither.
>> The contrast I tried to point out is that while we do have these
>> references in theory, and they are reflected by a refcount, too, we do
>> not actually have these references because the monitor does not yet have
>> a list of the BBs it owns.
> Oh yes, it has.  It's just outsources their actual storage to
> block-backend.c, but that's detail.

In my opinion it's not a reference made by the monitor, then, especially
because it's done through magic. With this interpretation,
block-backend.c considers every BB to be monitor-owned (until
blk_hide...() is called).

Also, if blk_backends is supposed to be the list of monitor-owned BBs,
then it's a really bad name and I put all the blame on that.

>> So it's not an "emphasize the verb because it may be contrary to your
>> expectations", but an "emphasize it because it is contrary to what the
>> current code does" (which is not actually referencing these BBs).
> I disagree :)

Then I'd say "It's contrary to my interpretation of what the current
code wants to do." Now it's my personal opinion! ;-)

I wouldn't mind removing the "does" from the commit message (obviously),
but that is not the only thing there which conflicts with your opinion.
It clearly states that blk_backends is not the list of monitor-owned
BlockBackends, whereas you are saying that it very much is.

So...? Rephrase it entirely? State that blk_backends is a bad name and
this commit is basically duplicating blk_backends as
monitor_block_backends, which is the correct name, and that a follow-up
commit will make blk_backends contain what it name implies it does? Or
just call the commit "Remove magic", because it adds explicit functions
for saying that a BB is monitor-owned or that it is not?


>> Like: It is supposed to have references. It says it does. But it
>> actually doesn't. It does "hold" them, however, because they are
>> accounted for in the BBs' refcounts.
> [...]

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]