Re: [PATCH 3/3] meson: Disable CONFIG_NOTIFY1 on FreeBSD

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Mon, 2024-02-05 at 15:31 +0000, Daniel P. Berrangé wrote:
> On Mon, Feb 05, 2024 at 08:23:41AM -0700, Warner Losh wrote:
> > On Wed, Jan 31, 2024 at 9:42 AM Daniel P. Berrangé
> > <berrange@redhat.com>
> > wrote:
> > 
> > > On Wed, Jan 31, 2024 at 05:24:10PM +0100, Philippe Mathieu-Daudé
> > > wrote:
> > > > Hi,
> > > > 
> > > > Warner, do you remember what this is about?
> > > > 
> > > > (
> > > https://cgit.freebsd.org/ports/commit/emulators/qemu-devel/files/patch-util_meson.build?id=2ab482e2c8f51eae7ffd747685b7f181fe1b3809
> > > > isn't very verbose).
> > > 
> > > That's simply going to workaround our incomplete feature
> > > check. We look for sys/inotify.h and if present, we
> > > assume that is in the C library. That's true on Linux,
> > > but not true on *BSD, hence the undefined symbol.
> > > 
> > > We need to augment the header file check with a linker
> > > symbol check for the C library.
> > > 
> > > If we wanted to also check for -linotify that'd make
> > > it portable to BSD, but not the behaviour difference
> > > mentioned below.
> > > 
> > > > 
> > > > On 25/1/24 20:48, Ilya Leoshkevich wrote:
> > > > > make vm-build-freebsd fails with:
> > > > > 
> > > > >      ld: error: undefined symbol: inotify_init1
> > > > >      >>> referenced by filemonitor-inotify.c:183
> > > (../src/util/filemonitor-inotify.c:183)
> > > > >      >>>
> > >  util_filemonitor-inotify.c.o:(qemu_file_monitor_new) in archive
> > > libqemuutil.a
> > > > > 
> > > > > On FreeBSD inotify functions are defined in libinotify.so, so
> > > > > it might
> > > > > be tempting to add it to the dependencies. Doing so, however,
> > > > > reveals
> > > > > that this library handles rename events differently from
> > > > > Linux:
> > > > > 
> > > > >      $ FILEMONITOR_DEBUG=1 build/tests/unit/test-util-
> > > > > filemonitor
> > > > >      Rename /tmp/test-util-filemonitor-K13LI2/fish/one.txt ->
> > > /tmp/test-util-filemonitor-K13LI2/two.txt
> > > > >      Event id=200000000 event=2 file=one.txt
> > > > >      Queue event id 200000000 event 2 file one.txt
> > > > >      Queue event id 100000000 event 2 file two.txt
> > > > >      Queue event id 100000002 event 2 file two.txt
> > > > >      Queue event id 100000000 event 0 file two.txt
> > > > >      Queue event id 100000002 event 0 file two.txt
> > > > >      Event id=100000000 event=0 file=two.txt
> > > > >      Expected event 0 but got 2
> > > 
> > > Interesting. So In the "Rename" test, the destination already
> > > exists.
> > > 
> > > BSD is thus reporting that 'two.txt' is deleted, before being
> > > (re)created
> > > Linux is only reporting 'two.txt' is created.
> > > 
> > > I don't think we can easily paper over this difference. The
> > > easiest is
> > > probably to conditionalize the test
> > > 
> > >  git diff
> > > diff --git a/tests/unit/test-util-filemonitor.c
> > > b/tests/unit/test-util-filemonitor.c
> > > index a22de27595..c3b2006365 100644
> > > --- a/tests/unit/test-util-filemonitor.c
> > > +++ b/tests/unit/test-util-filemonitor.c
> > > @@ -281,6 +281,14 @@ test_file_monitor_events(void)
> > >          { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > >            .filesrc = "one.txt", .watchid = &watch1,
> > >            .eventid = QFILE_MONITOR_EVENT_DELETED },
> > > +#ifdef __FreeBSD__
> > > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > > +          .filesrc = "two.txt", .watchid = &watch0,
> > > +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> > > +        { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > > +          .filesrc = "two.txt", .watchid = &watch2,
> > > +          .eventid = QFILE_MONITOR_EVENT_DELETED },
> > > +#endif
> > >          { .type = QFILE_MONITOR_TEST_OP_EVENT,
> > >            .filesrc = "two.txt", .watchid = &watch0,
> > >            .eventid = QFILE_MONITOR_EVENT_CREATED },
> > > 
> > 
> > I agree this is likely the best course of action. Has anybody filed
> > a bug
> > at https://bugs.freebsd.org?
> 
> I've not, and I'm not even sure I would class it a FreeBSD bug. Other
> than the fact that it differs from Linux behaviour, it feels like it
> is reasonble semantics to emit a 'delete' event in this scenario so
> that an event consumer can detect replacement of an existing file.
> 
> With regards,
> Daniel

Sounds reasonable; I will send a v2 with the meson adjustments and with
the test fix.