Skip to content

Commit

Permalink
Fix the inotify event handling in ReloadingYamlFileDisplayConfig::aut…
Browse files Browse the repository at this point in the history
…o_reload() (#3636)

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.
  • Loading branch information
Saviq authored Oct 23, 2024
2 parents 67c9fe3 + ea73246 commit 9812b80
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 22 deletions.
15 changes: 5 additions & 10 deletions src/miral/config_file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -188,24 +188,19 @@ void Watcher::handler(int) const
{
static size_t const sizeof_inotify_event = sizeof(inotify_event);

// A union ensures buffer is aligned for inotify_event
union
{
char buffer[sizeof_inotify_event + NAME_MAX + 1];
inotify_event unused [[maybe_unused]];
} inotify_magic;
alignas(inotify_event) char buffer[sizeof(inotify_event) + NAME_MAX + 1];

auto const readsize = read(inotify_fd, &inotify_magic, sizeof(inotify_magic));
auto const readsize = read(inotify_fd, buffer, sizeof(buffer));
if (readsize < static_cast<ssize_t>(sizeof_inotify_event))
{
return;
}

auto raw_buffer = inotify_magic.buffer;
while (raw_buffer != inotify_magic.buffer + readsize)
auto raw_buffer = buffer;
while (raw_buffer != buffer + readsize)
{
// This is safe because inotify_magic.buffer is aligned and event.len includes padding for alignment
auto& event = reinterpret_cast<inotify_event&>(*raw_buffer);
auto const& event = reinterpret_cast<inotify_event&>(*raw_buffer);
if (event.mask & (IN_CLOSE_WRITE | IN_MOVED_TO) && event.wd == directory_watch_descriptor.value())
try
{
Expand Down
31 changes: 19 additions & 12 deletions src/miral/static_display_config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -644,20 +644,24 @@ void miral::ReloadingYamlFileDisplayConfig::auto_reload()
if (auto const ml = the_main_loop())
{
ml->register_fd_handler({icf.value()}, this, [icf=icf.value(), this] (int)
{
union
{
inotify_event event;
char buffer[sizeof(inotify_event) + NAME_MAX + 1];
} inotify_buffer;
{
static size_t const sizeof_inotify_event = sizeof(inotify_event);

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

if (read(icf, &inotify_buffer, sizeof(inotify_buffer)) < static_cast<ssize_t>(sizeof(inotify_event)))
return;
auto const readsize = read(icf, buffer, sizeof(buffer));
if (readsize < static_cast<ssize_t>(sizeof_inotify_event))
return;

if (inotify_buffer.event.mask & (IN_CLOSE_WRITE | IN_CREATE | IN_MOVED_TO))
auto raw_buffer = buffer;
while (raw_buffer != buffer + readsize)
{
// This is safe because buffer is aligned and event.len includes padding for alignment
auto& event = reinterpret_cast<inotify_event&>(*raw_buffer);
if (event.mask & (IN_CLOSE_WRITE | IN_MOVED_TO))
try
{
if (inotify_buffer.event.name == basename)
if (event.name == basename)
{
std::lock_guard lock{mutex};
auto const& filename = config_path_.value() + "/" + basename;
Expand All @@ -671,7 +675,7 @@ void miral::ReloadingYamlFileDisplayConfig::auto_reload()
mir::log_warning("Failed to open display configuration: %s", filename.c_str());
}
}
else if (inotify_buffer.event.name == basename + layout_suffix)
else if (event.name == basename + layout_suffix)
{
check_for_layout_override();
}
Expand All @@ -691,7 +695,10 @@ void miral::ReloadingYamlFileDisplayConfig::auto_reload()
{
mir::log_warning("Failed to reload display configuration: %s", except.what());
}
});

raw_buffer += sizeof_inotify_event+event.len;
}
});
}
}
}
Expand Down

0 comments on commit 9812b80

Please sign in to comment.