Fix the inotify event handling in ReloadingYamlFileDisplayConfig::auto_reload()

[AlanGriffiths]

ReloadingYamlFileDisplayConfig::auto_reload() incorrectly triggered on IN_CREATE.

The reason for that is that IN_CREATE and IN_CLOSE_WRITE events can often be read as pairs and we were not handling multiple events correctly.

Note:

This is a backport of the logic in ConfigFile's Watcher::handler(). Ideally, we'd unify the code. But that's awkward as Watcher currently only handles changes to a single file.

It would make sense to rework Watcher to have a single watch on the configuration directory and register multiple files with it.

[tarek-y-ismail]

So here's what I understand you're doing:

1. Whenever the file descriptor gets written to, you read the data (event data) into a buffer that you ensure is aligned and has space for the maximum filename length
2. If the data read doesn't match the size of an inotify event, then some error has occured and we ignore this event
3. Otherwise, we continue on, reinterpreting the data we read as an event, and making sure its either a write or a move event. (create event caused issues with empty config files)
4. If the basename or a special suffixed basename are the subjects of the event, we handle the changes and update our display config
5. We apply the new display config to the connected displays
6. If there are other events, we handle them as well (not entirely sure about this one)

Is my understanding correct?

[AlanGriffiths]

3. Otherwise, we continue on, reinterpreting the data we read as an event, and making sure its either a write or a move event. (create event caused issues with empty config files)

We interpret the data we read as a series of events which we loop over.

The old code embodied a misconception that only a single event would be read. We never looked at the second or subsequent event in a series. That meant when writing a file (and getting an IN_CREATE, IN_CLOSE_WRITE pair) we were only seeing IN_CREATE, so that was (wrongly) added to the events that needed handling.

We don't want to try reading a file when it is created, because it may not have been written.

[RAOF]

This could instead be

alignas(inotify_event) char buffer[sizeof(inotify_event) + NAME_MAX + 1];

right?

[AlanGriffiths]

Yes, done

[RAOF]

Hm, done to one of the copies of this logic, but not to the one directly below here?

[RAOF]

We could be marking the inotify fd O_NONBLOCK and reading until we hit EAGAIN, but that'd just by a tiny optimisation.

Once the second union is replaced by alignas, this looks good to go.

(It also might be nice to deduplicate this logic into a helper, but there's only two copies at this point...)

[RAOF]

Hm, done to one of the copies of this logic, but not to the one directly below here?

[AlanGriffiths]

(It also might be nice to deduplicate this logic into a helper, but there's only two copies at this point...)

Agreed: #3645

In fact there ARE three copies - the original is in Frame (and has the same bug)

Fixed

[Saviq]

Once the second union is replaced by alignas, this looks good to go.