On Wed 31 Jan 2018 06:11:27 PM CET, Anton Nefedov wrote:
> On 31/1/2018 6:11 PM, Alberto Garcia wrote:
>> On Thu 18 Jan 2018 06:49:02 PM CET, Anton Nefedov wrote:
>>
>>> -static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest
>>> *self)
>>> +static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest
>>> *self,
>>> + bool nowait)
>>
>> It's a bit confusing to have a function called wait_foo() with a
>> parameter that says "don't wait"...
>>
>> How about
>>
>> check_serialising_requests(BdrvTrackedRequest *self, bool wait)
>>
>
> I think it might be more important to emphasize in the name that the
> function _might_ wait.
>
> i.e. it feels worse to read
> check_serialising_requests(req, true);
> when one needs to follow the function to find out that it might yield.
>
> Personally I'd vote for
>
> static int check_or_wait_serialising_requests(
> BdrvTrackedRequest *self, bool wait) {}
>
> and maybe even:
>
> static int check_serialising_requests(BdrvTrackedRequest *self) {
> return check_or_wait_serialising_requests(self, false);
>
> static int wait_serialising_requests(BdrvTrackedRequest *self) {
> return check_or_wait_serialising_requests(self, true);
> }
You're right. Either approach works for me though, also keeping the
current solution, wait_serialising_requests(req, true).
Berto
