[PATCH 0/3] make vm-build-freebsd fixes

[PATCH 0/3] make vm-build-freebsd fixes

[Ilya Leoshkevich]

Hi,

I needed to verify that my qemu-user changes didn't break BSD, and
Daniel Berrange suggested vm-build-freebsd on IRC. I had several
problems with it, which this series resolves.

Best regards,
Ilya

Ilya Leoshkevich (3):
  tests/vm: Set UseDNS=no in the sshd configuration
  tests/vm/freebsd: Reload the sshd configuration
  meson: Disable CONFIG_NOTIFY1 on FreeBSD

 meson.build        | 1 +
 tests/vm/basevm.py | 2 ++
 tests/vm/freebsd   | 2 ++
 3 files changed, 5 insertions(+)

-- 
2.43.0

[PATCH 1/3] tests/vm: Set UseDNS=no in the sshd configuration

[Ilya Leoshkevich]

make vm-build-freebsd sometimes fails with "Connection timed out during
banner exchange". The client strace shows:

    13:59:30 write(3, "SSH-2.0-OpenSSH_9.3\r\n", 21) = 21
    13:59:30 getpid()                       = 252655
    13:59:30 poll([{fd=3, events=POLLIN}], 1, 5000) = 1 ([{fd=3, revents=POLLIN}])
    13:59:32 read(3, "S", 1)                = 1
    13:59:32 poll([{fd=3, events=POLLIN}], 1, 3625) = 1 ([{fd=3, revents=POLLIN}])
    13:59:32 read(3, "S", 1)                = 1
    13:59:32 poll([{fd=3, events=POLLIN}], 1, 3625) = 1 ([{fd=3, revents=POLLIN}])
    13:59:32 read(3, "H", 1)                = 1

There is a 2s delay during connection, and ConnectTimeout is set to 1.
Raising it makes the issue go away, but we can do better. The server
truss shows:

    888: 27.811414714 socket(PF_INET,SOCK_DGRAM|SOCK_CLOEXEC,0) = 5 (0x5)
    888: 27.811765030 connect(5,{ AF_INET 10.0.2.3:53 },16) = 0 (0x0)
    888: 27.812166941 sendto(5,"\^Z/\^A\0\0\^A\0\0\0\0\0\0\^A2"...,39,0,NULL,0) = 39 (0x27)
    888: 29.363970743 poll({ 5/POLLRDNORM },1,5000) = 1 (0x1)

So the delay is due to a DNS query. Disable DNS queries in the server
config.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/vm/basevm.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 61725b83254..c0d62c08031 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -423,6 +423,8 @@ def console_ssh_init(self, prompt, user, pw):
     def console_sshd_config(self, prompt):
         self.console_wait(prompt)
         self.console_send("echo 'PermitRootLogin yes' >> /etc/ssh/sshd_config\n")
+        self.console_wait(prompt)
+        self.console_send("echo 'UseDNS no' >> /etc/ssh/sshd_config\n")
         for var in self.envvars:
             self.console_wait(prompt)
             self.console_send("echo 'AcceptEnv %s' >> /etc/ssh/sshd_config\n" % var)
-- 
2.43.0

[PATCH 2/3] tests/vm/freebsd: Reload the sshd configuration

[Ilya Leoshkevich]

After console_sshd_config(), the SSH server needs to be nudged to pick
up the new configs. The scripts for the other BSD flavors already do
this with a reboot, but a simple reload is sufficient.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/vm/freebsd | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index b581bd17fb7..9971bc1f2b2 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -108,6 +108,8 @@ class FreeBSDVM(basevm.BaseVM):
         prompt = "root@freebsd:~ #"
         self.console_ssh_init(prompt, "root", self._config["root_pass"])
         self.console_sshd_config(prompt)
+        self.console_wait(prompt)
+        self.console_send("service sshd reload\n")
 
         # setup virtio-blk #1 (tarfile)
         self.console_wait(prompt)
-- 
2.43.0

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

[Ilya Leoshkevich]

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

FreeBSD itself disables this functionality in the respective port [1].
So do it upstream too.

[1] https://cgit.freebsd.org/ports/tree/emulators/qemu-devel/files/patch-util_meson.build?id=984366c18f1bc54e38751afc29be08c596b83696

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index d0329966f1b..3d67d78b522 100644
--- a/meson.build
+++ b/meson.build
@@ -2403,6 +2403,7 @@ config_host_data.set('CONFIG_GETRANDOM',
 config_host_data.set('CONFIG_INOTIFY',
                      cc.has_header_symbol('sys/inotify.h', 'inotify_init'))
 config_host_data.set('CONFIG_INOTIFY1',
+                     host_os != 'freebsd' and
                      cc.has_header_symbol('sys/inotify.h', 'inotify_init1'))
 config_host_data.set('CONFIG_PRCTL_PR_SET_TIMERSLACK',
                      cc.has_header_symbol('sys/prctl.h', 'PR_SET_TIMERSLACK'))
-- 
2.43.0

Re: [PATCH 2/3] tests/vm/freebsd: Reload the sshd configuration

[Thomas Huth]

On 25/01/2024 20.48, Ilya Leoshkevich wrote:
> After console_sshd_config(), the SSH server needs to be nudged to pick
> up the new configs. The scripts for the other BSD flavors already do
> this with a reboot, but a simple reload is sufficient.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/vm/freebsd | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/vm/freebsd b/tests/vm/freebsd
> index b581bd17fb7..9971bc1f2b2 100755
> --- a/tests/vm/freebsd
> +++ b/tests/vm/freebsd
> @@ -108,6 +108,8 @@ class FreeBSDVM(basevm.BaseVM):
>           prompt = "root@freebsd:~ #"
>           self.console_ssh_init(prompt, "root", self._config["root_pass"])
>           self.console_sshd_config(prompt)
> +        self.console_wait(prompt)
> +        self.console_send("service sshd reload\n")

Could be simplified to:

	self.console_wait_send(prompt, "service sshd reload\n")

  Thomas

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

[Thomas Huth]

On 25/01/2024 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
> 
> FreeBSD itself disables this functionality in the respective port [1].
> So do it upstream too.
> 
> [1] https://cgit.freebsd.org/ports/tree/emulators/qemu-devel/files/patch-util_meson.build?id=984366c18f1bc54e38751afc29be08c596b83696
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index d0329966f1b..3d67d78b522 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2403,6 +2403,7 @@ config_host_data.set('CONFIG_GETRANDOM',
>   config_host_data.set('CONFIG_INOTIFY',
>                        cc.has_header_symbol('sys/inotify.h', 'inotify_init'))
>   config_host_data.set('CONFIG_INOTIFY1',
> +                     host_os != 'freebsd' and
>                        cc.has_header_symbol('sys/inotify.h', 'inotify_init1'))
>   config_host_data.set('CONFIG_PRCTL_PR_SET_TIMERSLACK',
>                        cc.has_header_symbol('sys/prctl.h', 'PR_SET_TIMERSLACK'))

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/3] tests/vm: Set UseDNS=no in the sshd configuration

[Thomas Huth]

On 25/01/2024 20.48, Ilya Leoshkevich wrote:
> make vm-build-freebsd sometimes fails with "Connection timed out during
> banner exchange". The client strace shows:
> 
>      13:59:30 write(3, "SSH-2.0-OpenSSH_9.3\r\n", 21) = 21
>      13:59:30 getpid()                       = 252655
>      13:59:30 poll([{fd=3, events=POLLIN}], 1, 5000) = 1 ([{fd=3, revents=POLLIN}])
>      13:59:32 read(3, "S", 1)                = 1
>      13:59:32 poll([{fd=3, events=POLLIN}], 1, 3625) = 1 ([{fd=3, revents=POLLIN}])
>      13:59:32 read(3, "S", 1)                = 1
>      13:59:32 poll([{fd=3, events=POLLIN}], 1, 3625) = 1 ([{fd=3, revents=POLLIN}])
>      13:59:32 read(3, "H", 1)                = 1
> 
> There is a 2s delay during connection, and ConnectTimeout is set to 1.
> Raising it makes the issue go away, but we can do better. The server
> truss shows:
> 
>      888: 27.811414714 socket(PF_INET,SOCK_DGRAM|SOCK_CLOEXEC,0) = 5 (0x5)
>      888: 27.811765030 connect(5,{ AF_INET 10.0.2.3:53 },16) = 0 (0x0)
>      888: 27.812166941 sendto(5,"\^Z/\^A\0\0\^A\0\0\0\0\0\0\^A2"...,39,0,NULL,0) = 39 (0x27)
>      888: 29.363970743 poll({ 5/POLLRDNORM },1,5000) = 1 (0x1)
> 
> So the delay is due to a DNS query. Disable DNS queries in the server
> config.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/vm/basevm.py | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 61725b83254..c0d62c08031 100644
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -423,6 +423,8 @@ def console_ssh_init(self, prompt, user, pw):
>       def console_sshd_config(self, prompt):
>           self.console_wait(prompt)
>           self.console_send("echo 'PermitRootLogin yes' >> /etc/ssh/sshd_config\n")
> +        self.console_wait(prompt)
> +        self.console_send("echo 'UseDNS no' >> /etc/ssh/sshd_config\n")
>           for var in self.envvars:
>               self.console_wait(prompt)
>               self.console_send("echo 'AcceptEnv %s' >> /etc/ssh/sshd_config\n" % var)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/3] make vm-build-freebsd fixes

[Thomas Huth]

On 25/01/2024 20.48, Ilya Leoshkevich wrote:
> Hi,
> 
> I needed to verify that my qemu-user changes didn't break BSD, and
> Daniel Berrange suggested vm-build-freebsd on IRC. I had several
> problems with it, which this series resolves.
> 
> Best regards,
> Ilya
> 
> Ilya Leoshkevich (3):
>    tests/vm: Set UseDNS=no in the sshd configuration
>    tests/vm/freebsd: Reload the sshd configuration
>    meson: Disable CONFIG_NOTIFY1 on FreeBSD
> 
>   meson.build        | 1 +
>   tests/vm/basevm.py | 2 ++
>   tests/vm/freebsd   | 2 ++
>   3 files changed, 5 insertions(+)

Tested-by: Thomas Huth <thuth@redhat.com>

I can take the patches through my tree (and fix the second patch to use 
console_wait_send() if you don't mind).

  Thomas

Re: [PATCH 0/3] make vm-build-freebsd fixes

[Ilya Leoshkevich]

On Wed, 2024-01-31 at 14:24 +0100, Thomas Huth wrote:
> On 25/01/2024 20.48, Ilya Leoshkevich wrote:
> > Hi,
> > 
> > I needed to verify that my qemu-user changes didn't break BSD, and
> > Daniel Berrange suggested vm-build-freebsd on IRC. I had several
> > problems with it, which this series resolves.
> > 
> > Best regards,
> > Ilya
> > 
> > Ilya Leoshkevich (3):
> >    tests/vm: Set UseDNS=no in the sshd configuration
> >    tests/vm/freebsd: Reload the sshd configuration
> >    meson: Disable CONFIG_NOTIFY1 on FreeBSD
> > 
> >   meson.build        | 1 +
> >   tests/vm/basevm.py | 2 ++
> >   tests/vm/freebsd   | 2 ++
> >   3 files changed, 5 insertions(+)
> 
> Tested-by: Thomas Huth <thuth@redhat.com>
> 
> I can take the patches through my tree (and fix the second patch to
> use 
> console_wait_send() if you don't mind).
> 
>   Thomas
> 

Sure, that sounds good to me. Thanks!

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

[Philippe Mathieu-Daudé]

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).

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

Wouldn't it be better to use a runtime check in qemu_file_monitor_new()?

> FreeBSD itself disables this functionality in the respective port [1].
> So do it upstream too.
> 
> [1] https://cgit.freebsd.org/ports/tree/emulators/qemu-devel/files/patch-util_meson.build?id=984366c18f1bc54e38751afc29be08c596b83696
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index d0329966f1b..3d67d78b522 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2403,6 +2403,7 @@ config_host_data.set('CONFIG_GETRANDOM',
>   config_host_data.set('CONFIG_INOTIFY',
>                        cc.has_header_symbol('sys/inotify.h', 'inotify_init'))
>   config_host_data.set('CONFIG_INOTIFY1',
> +                     host_os != 'freebsd' and
>                        cc.has_header_symbol('sys/inotify.h', 'inotify_init1'))
>   config_host_data.set('CONFIG_PRCTL_PR_SET_TIMERSLACK',
>                        cc.has_header_symbol('sys/prctl.h', 'PR_SET_TIMERSLACK'))

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

[Daniel P. Berrangé]

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 },


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

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?

Warner

[-- Attachment #2: Type: text/html, Size: 4584 bytes --]

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Ilya Leoshkevich]

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.

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

[Kyle Evans]

On 2/5/24 09:31, 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:
 > [... snip ...]
>>>> 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.
> 

FWIW, +1... unless we miss the follow-up notification that it's been 
created, I'd personally put it into the WONTFIX bucket pretty quickly.

Barring some kind of NOTE_COVER (bad name) that can be emitted if a file 
is simply replaced by another, a directory being reported as shortened 
then extended is a valid and useful representation of the situation 
(even if not completely accurate) to avoid consumers missing the action 
entirely.

Thanks,

Kyle Evans