Re: [PATCH v2 06/36] block: BdrvChildClass: add .get_parent_aio_context handler - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org, armbru@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v2 06/36] block: BdrvChildClass: add .get_parent_aio_context handler
Date: Fri, 22 Jan 2021 14:04:55 +0300	[thread overview]
Message-ID: <fb41291e-45c3-31d6-3f56-5ef5c42feaf3@virtuozzo.com> (raw)
In-Reply-To: <20210119163805.GI5012@merkur.fritz.box>

19.01.2021 19:38, Kevin Wolf wrote:
> Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 18.01.2021 18:13, Kevin Wolf wrote:
>>> Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Add new handler to get aio context and implement it in all child
>>>> classes. Add corresponding public interface to be used soon.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> Hm, are you going to introduce cases where parent and child context
>>> don't match, or why is this a useful function?
>>>
>>> Even if so, I feel it shouldn't be any of the child's business what
>>> AioContext the parent uses.
>>>
>>> Well, maybe the rest of the series will answer this.
>>
>> It's for the following patch, to not pass parent (as opaque) with it's
>> class, and with its ctx in separate. Just to simplify the interface of
>> the function, we are going to work with a lot.
> 
> In a way, the result is nicer because we already assume that ctx is the
> parent context when we move things to different AioContexts. So it's
> more consistent to just directly take it from the parent.
> 
> At the same time, it also complicates things a bit because now we need a
> defined state in the middle of an operation that adds, removes or
> replaces a child.
> 
> I guess I still to make more progress in the review of this series until
> I see how you're using the interface.
> 
>> Hm. I'll take this opportunity to say, that the terminology that calls
>> graph edge "BdrvChild" always leads to the mess between parents and
>> children.. "child_class" is a class of parent.. list of parents is
>> list of children... It all would be a lot simpler to understand if
>> BdrvChild was BdrvEdge or something like this.
> 
> Yeah, that would probably be clearer now that we actually use it to
> work with both ends of the edge. And BdrvNode instead of
> BlockDriverState, I guess.

Do you think, we can just rename them? Or it would be too painful for developers,
who get used to current names? I can make patches

-- 
Best regards,
Vladimir