[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: Tue, 23 Feb 2016 14:52:41 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

On 23.02.2016 10:48, Markus Armbruster wrote:
> Max Reitz <address@hidden> writes:
>> 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).
> block-backend.c holds a reference from blk_backends.  By *why* does it
> do that?  Let's go through the uses of blk_backends.
> 0. blk_backends maintenance: blk_new(), blk_delete(),
>    blk_hide_on_behalf_of_hmp_drive_del()
> 1. To permit lookup by name, with blk_by_name().
>    In my view, block-backend.c holds this reference in trust for those
>    parts of QEMU that reference by name rather than by pointer, most
>    prominently the monitor.
>    I can't see the point of backing up the reference by name with a
>    reference by pointer in the monitor, like your patch seems to do.
>    What's the difference between the two?  Can one ever exist without
>    the other?  Why in the monitor, and not in any other part looking up
>    by name?
> 2. To iterate over all named ones, with blk_next().
>    Since this can only return named BBs, the reference held in trust for
>    named lookup suffices.
> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
>    Since this must only return named BBs, the reference held in trust
>    for named lookup suffices.
> 4. To do something so unimportant that it's not worth explaining, with
>    blk_remove_bs().
>    I made a point of giving every single external function a carefully
>    worded contract, to hopefully shame future contributors into
>    following suit.  Didn't work.

Side note: I consider it very important and there was no other way to do
it before this series, because there is no list of all block backends.

>> 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.
> Naming is hard.  Feel free to propose better comments and/or better
> names.

I did. It's "monitor_block_backends".

>>>> 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?
> Can you explain the *actual* difference between blk_backends and
> monitor_block_backends?  "Actual" in the sense of difference in contents
> over time.

First difference: The name. That's enough reason for me.

Second difference: blk_new() adds any BB to blk_backends automatically.
It doesn't do so for monitor_block_backends.

Third difference: Often the monitor doesn't take care of signalling that
it's releasing its reference, only sometimes (where "sometimes" means
every time blk_hide...() is called). blk_delete() will instead
automatically remove it from blk_backends, because it's assuming that
the last reference had been held by the monitor.

The second and third difference are what I referred to as "magic". You
could also call them "convenience", if you prefer, but in any case as we
can see by the existence blk_hide...(), removing the monitor reference
in blk_delete() appears to be wrong. Both should be separate operations,
and this is what this patch does.

Assuming that any blk_new() is ultimately done by the monitor, we maybe
actually do not need an own monitor_add_blk() function; except that
Kevin stated that he does deem it useful when I proposed inlining it
(back) into blk_new().

All in all, you have convinced me that the commit message is incorrect.
It should state that blk_backends is effectively repurposed to contain
the list of all BBs, and that a more explicit monitor_block_backends
list will take its place, with explicit operations for the monitor to
signal when it takes or releases the reference to a BB.

However, I still believe this patch itself to be useful.


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

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