Markus Armbruster <arm...@redhat.com> writes:
> Fabiano Rosas <faro...@suse.de> writes:
>
>> Markus Armbruster <arm...@redhat.com> writes:
>>
>>> Fabiano Rosas <faro...@suse.de> writes:
>>>
>>>> We're about to enable the use of O_DIRECT in the migration code and
>>>> due to the alignment restrictions imposed by filesystems we need to
>>>> make sure the flag is only used when doing aligned IO.
>>>>
>>>> The migration will do parallel IO to different regions of a file, so
>>>> we need to use more than one file descriptor. Those cannot be obtained
>>>> by duplicating (dup()) since duplicated file descriptors share the
>>>> file status flags, including O_DIRECT. If one migration channel does
>>>> unaligned IO while another sets O_DIRECT to do aligned IO, the
>>>> filesystem would fail the unaligned operation.
>>>>
>>>> The add-fd QMP command along with the fdset code are specifically
>>>> designed to allow the user to pass a set of file descriptors with
>>>> different access flags into QEMU to be later fetched by code that
>>>> needs to alternate between those flags when doing IO.
>>>>
>>>> Extend the fdset matching function to behave the same with the
>>>> O_DIRECT flag.
>>>>
>>>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>>>> ---
>>>> monitor/fds.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/monitor/fds.c b/monitor/fds.c
>>>> index 9a28e4b72b..42bf3eb982 100644
>>>> --- a/monitor/fds.c
>>>> +++ b/monitor/fds.c
>>>> @@ -413,6 +413,12 @@ static bool monitor_fdset_flags_match(int flags, int
>>>> fd_flags)
>>> static bool monitor_fdset_flags_match(int flags, int fd_flags)
>>> {
>>> bool match = false;
>>>
>>>> if ((flags & O_ACCMODE) == (fd_flags & O_ACCMODE)) {
>>>> match = true;
>>>> +
>>>> +#ifdef O_DIRECT
>>>> + if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) {
>>>> + match = false;
>>>> + }
>>>> +#endif
>>>> }
>>>>
>>>> return match;
>>> }
>>>
>>> I'd prefer something like
>>>
>>> static bool monitor_fdset_flags_match(int flags, int fd_flags)
>>> {
>>> #ifdef O_DIRECT
>>> if ((flags & O_DIRECT) != (fd_flags & O_DIRECT)) {
>>> return false;
>>> }
>>> #endif
>>>
>>> if ((flags & O_ACCMODE) != (fd_flags & O_ACCMODE)) {
>>> return false;
>>>
>>> }
>>>
>>> return true;
>>> }
>>
>> This makes the O_DIRECT flag dictate the outcome when it's present. I
>> want O_DIRECT to be considered only when all other flags have matched.
>>
>> Otherwise we regress the original use-case if the user happened to have
>> put O_DIRECT in the flags. A non-match due to different O_ACCMODE would
>> become a match due to (possibly) matching O_DIRECT.
>
> The fact that I missed this signifies one of two things: either was
> suffering from code review brain (quite possible!), or this needs a
> comment and/or clearer coding.
>
> If I understand you correctly, you want to return true when the bits
> selected by the two masks together match.
>
> If we didn't need ifdeffery, we wouldn't use nested conditionals for
> comparing bits under a mask. We'd use something like
>
> int mask = O_ACCMODE | O_DIRECT;
>
> return (flags & mask) == (fd_flags & mask);
>
> Bring back the ifdeffery:
>
> int mask = O_ACCMODE;
>
> #ifdef O_DIRECT
> mask |= O_DIRECT;
> #endif
>
> return (flags & mask) == (fd_flags & mask);
Could be. I'll change it.
>
> Or maybe even
>
> #ifndef O_DIRECT
> #define O_DIRECT 0
> #endif
>
> int mask = O_ACCMODE | O_DIRECT;
>
> return (flags & mask) == (fd_flags & mask);
>
> Not sure this is even worth a helper function.
Agreed.
>
> Or am I stull suffering from code review brain?
Yes, stull suffering. =)
