Re: [PATCH v2] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout

[Nir Soffer]

_clusters):
> +if bitmap_test(l2_bitmap, idx):
> +if skip > 0:
> +reader.seek(cluster_size * skip, 1)
> +skip = 0
> +cluster = reader.read(cluster_size)
> +# If the last cluster is smaller than cluster_size pad it 
> with zeroes
> +if len(cluster) < cluster_size:
> +cluster += bytes(cluster_size - len(cluster))

This can be done only for the last cluster.

> +sys.stdout.buffer.write(cluster)
> +else:
> +skip += 1

If would be easier to work with if you add a function iterating over the
l2_entries, yielding the he cluster index to copy:

  def iter_l2_entries(bitmap, clusters):
for idx in range(clusters):
  if bitmap_test(bitmap, idx):
yield idx

The copy loop can read using os.pread():

for idx in iter_l2_entries(l2_bitmap, total_data_clusters):
cluster = os.pread(fd, cluster_size, cluster_size * idx)
sys.stdout.buffer.write(cluster)

I'm not sure the offset is right in my example, it is hard to
understand the semantics
of skip in your code.

> +
> +
> +if __name__ == "__main__":

Usually code is easier to work with when __main__  calls main().

For example:

  def main():
args = parse_args()
validate args...
actual work...

  if __name__ == '__main__':
main()

This does not leak unwanted global variables from code parsing
arguments, and we can test any function
if needed. This is also more consistent since we can write any script
using this style.

> +# Command-line arguments
> +parser = argparse.ArgumentParser(

parser is global - is this intended?

> +description="This program converts a QEMU disk image to qcow2 "
> +"and writes it to the standard output"
> +)
> +parser.add_argument("input_file", help="name of the input file")
> +parser.add_argument(
> +"-f",
> +dest="input_format",
> +metavar="input_format",
> +default="raw",
> +help="format of the input file (default: raw)",
> +)
> +parser.add_argument(
> +"-c",
> +dest="cluster_size",
> +metavar="cluster_size",
> +help=f"qcow2 cluster size (default: {QCOW2_DEFAULT_CLUSTER_SIZE})",
> +default=QCOW2_DEFAULT_CLUSTER_SIZE,
> +type=int,
> +choices=[1 << x for x in range(9, 22)],
> +)
> +parser.add_argument(
> +"-r",
> +dest="refcount_bits",
> +metavar="refcount_bits",
> +help=f"width of the reference count entries (default: 
> {QCOW2_DEFAULT_REFCOUNT_BITS})",
> +default=QCOW2_DEFAULT_REFCOUNT_BITS,
> +type=int,
> +choices=[1 << x for x in range(7)],
> +)
> +parser.add_argument(
> +"-v",
> +dest="qcow2_version",
> +metavar="qcow2_version",
> +help=f"qcow2 version (default: {QCOW2_DEFAULT_VERSION})",
> +default=QCOW2_DEFAULT_VERSION,
> +type=int,
> +choices=[2, 3],
> +)
> +args = parser.parse_args()

args is global, is this intended?

> +
> +if not os.path.isfile(args.input_file):
> +sys.exit(f"[Error] {args.input_file} does not exist or is not a 
> regular file.")
> +
> +if args.refcount_bits != 16 and args.qcow2_version != 3:
> +sys.exit(f"[Error] refcount_bits={args.refcount_bits} is only 
> supported with qcow2_version=3.")
> +
> +if sys.stdout.isatty():
> +sys.exit("[Error] Refusing to write to a tty. Try redirecting 
> stdout.")
> +
> +with get_input_as_raw_file(args.input_file, args.input_format) as 
> raw_file:

raw_file is global, is this intended?

> +create_qcow2_file(

The name does not reflect what the function does - this does not
create a qcow2 file, but write
a qcow2 content to stdout. Maybe write_qcow2_content()?

> +raw_file,
> +args.cluster_size,
> +args.refcount_bits,
> +args.qcow2_version,
> +)
> --
> 2.39.2
>
>

Nir

Re: [PATCH v2] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout

[Nir Soffer]

_clusters):
> +if bitmap_test(l2_bitmap, idx):
> +if skip > 0:
> +reader.seek(cluster_size * skip, 1)
> +skip = 0
> +cluster = reader.read(cluster_size)
> +# If the last cluster is smaller than cluster_size pad it 
> with zeroes
> +if len(cluster) < cluster_size:
> +cluster += bytes(cluster_size - len(cluster))

This can be done only for the last cluster.

> +sys.stdout.buffer.write(cluster)
> +else:
> +skip += 1

If would be easier to work with if you add a function iterating over the
l2_entries, yielding the he cluster index to copy:

  def iter_l2_entries(bitmap, clusters):
for idx in range(clusters):
  if bitmap_test(bitmap, idx):
yield idx

The copy loop can read using os.pread():

for idx in iter_l2_entries(l2_bitmap, total_data_clusters):
cluster = os.pread(fd, cluster_size, cluster_size * idx)
sys.stdout.buffer.write(cluster)

I'm not sure the offset is right in my example, it is hard to
understand the semantics
of skip in your code.

> +
> +
> +if __name__ == "__main__":

Usually code is easier to work with when __main__  calls main().

For example:

  def main():
args = parse_args()
validate args...
actual work...

  if __name__ == '__main__':
main()

This does not leak unwanted global variables from code parsing
arguments, and we can test any function
if needed. This is also more consistent since we can write any script
using this style.

> +# Command-line arguments
> +parser = argparse.ArgumentParser(

parser is global - is this intended?

> +description="This program converts a QEMU disk image to qcow2 "
> +"and writes it to the standard output"
> +)
> +parser.add_argument("input_file", help="name of the input file")
> +parser.add_argument(
> +"-f",
> +dest="input_format",
> +metavar="input_format",
> +default="raw",
> +help="format of the input file (default: raw)",
> +)
> +parser.add_argument(
> +"-c",
> +dest="cluster_size",
> +metavar="cluster_size",
> +help=f"qcow2 cluster size (default: {QCOW2_DEFAULT_CLUSTER_SIZE})",
> +default=QCOW2_DEFAULT_CLUSTER_SIZE,
> +type=int,
> +choices=[1 << x for x in range(9, 22)],
> +)
> +parser.add_argument(
> +"-r",
> +dest="refcount_bits",
> +metavar="refcount_bits",
> +help=f"width of the reference count entries (default: 
> {QCOW2_DEFAULT_REFCOUNT_BITS})",
> +default=QCOW2_DEFAULT_REFCOUNT_BITS,
> +type=int,
> +choices=[1 << x for x in range(7)],
> +)
> +parser.add_argument(
> +"-v",
> +dest="qcow2_version",
> +metavar="qcow2_version",
> +help=f"qcow2 version (default: {QCOW2_DEFAULT_VERSION})",
> +default=QCOW2_DEFAULT_VERSION,
> +type=int,
> +choices=[2, 3],
> +)
> +args = parser.parse_args()

args is global, is this intended?

> +
> +if not os.path.isfile(args.input_file):
> +sys.exit(f"[Error] {args.input_file} does not exist or is not a 
> regular file.")
> +
> +if args.refcount_bits != 16 and args.qcow2_version != 3:
> +sys.exit(f"[Error] refcount_bits={args.refcount_bits} is only 
> supported with qcow2_version=3.")
> +
> +if sys.stdout.isatty():
> +sys.exit("[Error] Refusing to write to a tty. Try redirecting 
> stdout.")
> +
> +with get_input_as_raw_file(args.input_file, args.input_format) as 
> raw_file:

raw_file is global, is this intended?

> +create_qcow2_file(

The name does not reflect what the function does - this does not
create a qcow2 file, but write
a qcow2 content to stdout. Maybe write_qcow2_content()?

> +raw_file,
> +args.cluster_size,
> +args.refcount_bits,
> +args.qcow2_version,
> +)
> --
> 2.39.2
>
>

Nir

[Acme] Re: Presentations for the ACME session at IETF 120

[Yoav Nir]

Very well.  You are on the agenda.  We’ll leave a generic title slide for  you 
in the chair deck.

> On 25 Jul 2024, at 16:24, Sipos, Brian J.  wrote:
> 
> Yoav,
> I did not have enough time to approve specific slides for this week, but I 
> can talk about the status of the DTN-related ACME draft for 10-15 minutes.
>  
> From: Yoav Nir mailto:ynir.i...@gmail.com>> 
> Sent: Wednesday, July 24, 2024 7:52 PM
> To: IETF ACME mailto:acme@ietf.org>>
> Subject: [EXT] [Acme] Re: Presentations for the ACME session at IETF 120
>  
> APL external email warning: Verify sender forwardingalgori...@ietf.org 
> <mailto:forwardingalgori...@ietf.org> before clicking links or attachments
>  
> 
> Hi, all
> 
> So far, we've only got slides from Aaron. And while it's be happy to hear him 
> talk for two hours, he insists that he needs far less than that. 
> 
> Please send or propose your slides real soon. 
> 
> Regards, 
> 
> Tomofumi and Yoav 
> On Jul 21, 2024, at 19:26, Yoav Nir  <mailto:ynir.i...@gmail.com>> wrote:
> Hi, all
>  
> I see now that we have neglected to ask for presentations.
>  
> We’ll treat Brian’s email from Friday [1] and Aaron’s message from last week 
> [2] as presentation requests.
>  
> Anyone else, please let us know.  And also, getting the slides sooner is 
> better than later.
>  
> Apologies for sending this out so late,
>  
> Tomofumi and Yoav
>  
>  
> [1] https://mailarchive.ietf.org/arch/msg/acme/msWRY-yxAX_HL2IS4UjgxRFllSI/
> [2] https://mailarchive.ietf.org/arch/msg/acme/7OtsBq6ZsHuXUMuHYwb7LwEaBPY/

___
Acme mailing list -- acme@ietf.org
To unsubscribe send an email to acme-le...@ietf.org

[Acme] Re: Presentations for the ACME session at IETF 120

[Yoav Nir]

Hi, all

So far, we've only got slides from Aaron. And while it's be happy to hear him 
talk for two hours, he insists that he needs far less than that.

Please send or propose your slides real soon.

Regards,

Tomofumi and Yoav


On Jul 21, 2024, 19:26, at 19:26, Yoav Nir  wrote:
>Hi, all
>
>I see now that we have neglected to ask for presentations.
>
>We’ll treat Brian’s email from Friday [1] and Aaron’s message from last
>week [2] as presentation requests.
>
>Anyone else, please let us know.  And also, getting the slides sooner
>is better than later.
>
>Apologies for sending this out so late,
>
>Tomofumi and Yoav
>
>
>[1]
>https://mailarchive.ietf.org/arch/msg/acme/msWRY-yxAX_HL2IS4UjgxRFllSI/
>[2]
>https://mailarchive.ietf.org/arch/msg/acme/7OtsBq6ZsHuXUMuHYwb7LwEaBPY/
___
Acme mailing list -- acme@ietf.org
To unsubscribe send an email to acme-le...@ietf.org

Re: Strange behavior of qemu-img map: zero/data status depends on fallocated image page cache content

[Nir Soffer]

On Sun, Jun 30, 2024 at 5:31 PM Nir Soffer  wrote:
>
> I found a strange behavior in qemu-img map - zero/data status depends on page
> cache content.  It looks like a kernel issue since qemu-img map is using
> SEEK_HOLE/DATA (block/file-posix.c line 3111).
>
> Tested with latest qemu on kernel 6.9.6-100.fc39.x86_64. I see similar 
> behavior
> in xfs and ex4 filesystems.
>
> After creating a allocated image:
>
> # qemu-img create -f raw -o preallocation=falloc falloc.img 1g
> Formatting 'falloc.img', fmt=raw size=1073741824 preallocation=falloc
>
> qemu-img map reports the image as sparse (expect the first block which we 
> fully
> allocate):
>
> # qemu-img map --output json falloc.img
> [{ "start": 0, "length": 4096, "depth": 0, "present": true,
> "zero": false, "data": true, "offset": 0},
> { "start": 4096, "length": 1073737728, "depth": 0, "present":
> true, "zero": true, "data": false, "offset": 4096}]
>
> This is goo for copy or read performance, since we can skip reading the areas
> with data=false, but on the other hand this is bad for correctness, since we
> cannot preserve the allocation of the entire image, since it look like a 
> sparse
> image:
>
> # qemu-img create -f raw sparse.img 1g
> Formatting 'sparse.img', fmt=raw size=1073741824
>
> # qemu-img map --output json sparse.img
> [{ "start": 0, "length": 4096, "depth": 0, "present": true,
> "zero": false, "data": true, "offset": 0},
> { "start": 4096, "length": 1073737728, "depth": 0, "present":
> true, "zero": true, "data": false, "offset": 4096}]
>
> But look what happens when we get some of the image into the page cache:
>
> # dd if=falloc.img bs=1M count=512 of=/dev/null
>
> # qemu-img map --output json falloc.img
> [{ "start": 0, "length": 544210944, "depth": 0, "present": true,
> "zero": false, "data": true, "offset": 0},
> { "start": 544210944, "length": 529530880, "depth": 0, "present":
> true, "zero": true, "data": false, "offset": 544210944}]
>
> Now half of the image is reported as data=true and half as data=false. If we
> read the entire image all of it is reported as data=true:
>
> # dd if=falloc.img bs=1M count=1024 of=/dev/null
>
> # qemu-img map --output json falloc.img
> [{ "start": 0, "length": 1073741824, "depth": 0, "present": true,
> "zero": false, "data": true, "offset": 0}]
>
> If we drop caches, the image go back to the initial state (almost):
>
> # sync; echo 1 > /proc/sys/vm/drop_caches
>
> # qemu-img map --output json falloc.img
> [{ "start": 0, "length": 16384, "depth": 0, "present": true,
> "zero": false, "data": true, "offset": 0},
> { "start": 16384, "length": 1073725440, "depth": 0, "present":
> true, "zero": true, "data": false, "offset": 16384}]
>
> Based on the lseek(2) the file system can do anything, but the page
> cache is not mentioned
> as something that may affect the result of the call:
>
>Seeking file data and holes
>Since  Linux  3.1,  Linux  supports the following additional values for
>whence:
>
>SEEK_DATA
>   Adjust the file offset to the next location in the file  greater
>   than  or  equal  to offset containing data.  If offset points to
>   data, then the file offset is set to offset.
>
>SEEK_HOLE
>   Adjust the file offset to the next hole in the file greater than
>   or equal to offset.  If offset points into the middle of a hole,
>   then the file offset is set to offset.  If there is no hole past
>   offset, then the file offset is adjusted to the end of the  file
>   (i.e., there is an implicit hole at the end of any file).
>
>In both of the above cases, lseek() fails if offset points past the end
>of the file.
>
>These  operations  allow  applications to map holes in a sparsely allo‐
>cated file.  This can be useful for applications such  as  file  backup
>tools,  which  can save space when creating backups and preserve holes,
>  

Re: backup-discard-source iotest fails on xfs, pass on ext4

[Nir Soffer]

On Sat, Jun 29, 2024 at 12:17 AM Nir Soffer  wrote:
>
> While testing 
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00644.html
> I noticed that
> this tests is failing also with master
> (28b8a57ad63670aa0ce90334523dc552b13b4336).
>
> The test succeeds with ext4, and fail with xfs.

Are we ok with this broken test?

> Nir
>
> ---
>
> Success on ext4:
>
> $ TEST_DIR=/data/tetests/qemu-iotests/check backup-discard-source -qcow2
> test-ext4/ test-xfs/
> [nsoffer build (master)]$ TEST_DIR=/data/test-ext4/scratch
> tests/qemu-iotests/check backup-discard-source -qcow2
> QEMU  -- "/home/nsoffer/src/qemu/build/qemu-system-x86_64"
> -nodefaults -display none -accel qtest
> QEMU_IMG  -- "/home/nsoffer/src/qemu/build/qemu-img"
> QEMU_IO   -- "/home/nsoffer/src/qemu/build/qemu-io" --cache
> writeback --aio threads -f qcow2
> QEMU_NBD  -- "/home/nsoffer/src/qemu/build/qemu-nbd"
> IMGFMT-- qcow2
> IMGPROTO  -- file
> PLATFORM  -- Linux/x86_64 sparse.local 6.9.4-100.fc39.x86_64
> TEST_DIR  -- /data/test-ext4/scratch
> SOCK_DIR  -- /tmp/qemu-iotests-rdoofjdc
> GDB_OPTIONS   --
> VALGRIND_QEMU --
> PRINT_QEMU_OUTPUT --
>
> backup-discard-source   pass   [00:14:09] [00:14:10]   0.8s
> Passed all 1 iotests
>
>
> Failing on xfs:
>
> $ TEST_DIR=/data/test-xfs/scratch tests/qemu-iotests/check
> backup-discard-source -qcow2
> QEMU  -- "/home/nsoffer/src/qemu/build/qemu-system-x86_64"
> -nodefaults -display none -accel qtest
> QEMU_IMG  -- "/home/nsoffer/src/qemu/build/qemu-img"
> QEMU_IO   -- "/home/nsoffer/src/qemu/build/qemu-io" --cache
> writeback --aio threads -f qcow2
> QEMU_NBD  -- "/home/nsoffer/src/qemu/build/qemu-nbd"
> IMGFMT-- qcow2
> IMGPROTO  -- file
> PLATFORM  -- Linux/x86_64 sparse.local 6.9.4-100.fc39.x86_64
> TEST_DIR  -- /data/test-xfs/scratch
> SOCK_DIR  -- /tmp/qemu-iotests-4j_gu404
> GDB_OPTIONS   --
> VALGRIND_QEMU --
> PRINT_QEMU_OUTPUT --
>
> backup-discard-source   fail   [00:15:44] [00:15:45]   0.8s
> (last: 0.8s)  failed, exit status 1
> --- /home/nsoffer/src/qemu/tests/qemu-iotests/tests/backup-discard-source.out
> +++ 
> /data/test-xfs/scratch/qcow2-file-backup-discard-source/backup-discard-source.out.bad
> @@ -1,5 +1,14 @@
> -..
> +F.
> +==
> +FAIL: test_discard_cbw (__main__.TestBackup.test_discard_cbw)
> +1. do backup(discard_source=True), which should inform
> +--
> +Traceback (most recent call last):
> +  File 
> "/home/nsoffer/src/qemu/tests/qemu-iotests/tests/backup-discard-source",
> line 147, in test_discard_cbw
> +self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
> +AssertionError: 1249280 not less than 524288
> +
>  --
>  Ran 2 tests
>
> -OK
> +FAILED (failures=1)
> Failures: backup-discard-source
> Failed 1 of 1 iotests
>
>
> $ xfs_info /data/test-xfs/
> meta-data=/dev/mapper/data-test--xfs isize=512agcount=4, agsize=655360 
> blks
>  =   sectsz=512   attr=2, projid32bit=1
>  =   crc=1finobt=1, sparse=1, rmapbt=0
>  =   reflink=1bigtime=1 inobtcount=1 nrext64=0
> data =   bsize=4096   blocks=2621440, imaxpct=25
>  =   sunit=0  swidth=0 blks
> naming   =version 2  bsize=4096   ascii-ci=0, ftype=1
> log  =internal log   bsize=4096   blocks=16384, version=2
>  =   sectsz=512   sunit=0 blks, lazy-count=1
> realtime =none   extsz=4096   blocks=0, rtextents=0

[Acme] Presentations for the ACME session at IETF 120

[Yoav Nir]

Hi, all

I see now that we have neglected to ask for presentations.

We’ll treat Brian’s email from Friday [1] and Aaron’s message from last week 
[2] as presentation requests.

Anyone else, please let us know.  And also, getting the slides sooner is better 
than later.

Apologies for sending this out so late,

Tomofumi and Yoav


[1] https://mailarchive.ietf.org/arch/msg/acme/msWRY-yxAX_HL2IS4UjgxRFllSI/
[2] https://mailarchive.ietf.org/arch/msg/acme/7OtsBq6ZsHuXUMuHYwb7LwEaBPY/___
Acme mailing list -- acme@ietf.org
To unsubscribe send an email to acme-le...@ietf.org

Re: consistent naming for tests

[Nir Lisker]

Sounds good. Maybe also add that junit 5 should be used and 4 is just
legacy.

Note that the wiki also has instructions related to tests, but I think it's
just for running them.

On Wed, Jul 10, 2024, 10:25 Johan Vos  wrote:

> Thanks all for commenting.
> What I have read so far seems that there is an agreement for this approach:
> * don't prefix tests with `test` anymore
> * use a (somehow) descriptive name
> * add a comment that refers to the JBS issue that this test is dealing with
> * (optional) in case the test or test scenario is complex, add a comment
> that briefly describes what is being tested.
>
> If that is in line with what most people want, I can create a PR to add
> this to the CONTRIBUTING.md file.
>
> - Johan
>
> On Wed, Jul 10, 2024 at 1:36 AM Nir Lisker  wrote:
>
>> * in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
>>> * in some cases, tests have a concise but somehow meaningful name (e.g.
>>> `testScrollBarStaysVisible`)
>>
>>
>> Prefixing 'test' was an old convention for testing frameworks. I have
>> been dropping that prefix in my projects since I'm in a test
>> class/package/source folder anyway, and it's not like there're methods in a
>> test class that aren't used for testing. I also use long descriptive names,
>> like 'newValueNotSetIfOldValueWasInvalid()' or, alternatively,
>> 'doNotSetNewValueIfOldValueWasInvalid()'. John's nesting names are also
>> good when nesting is appropriate.
>>
>> * in some cases, tests refer to JBS issues (e.g. testJDK8309935)
>>
>> * in some cases, the test is explained in comments.
>>
>>
>> I don't like JBS numbers as names, but I like them as links in a comment.
>> I prefer the name of the test and methods to be self-explanatory, like in
>> non-test code, rather than comments. However, sometimes comments are needed
>> because of tricky or non-trivial situations, which is part of what tests
>> are for.
>>
>>
>> On Tue, Jul 9, 2024 at 6:30 PM Kevin Rushforth <
>> kevin.rushfo...@oracle.com> wrote:
>>
>>> This might be a combination of Eclipse and eCryptfs. I agree that 143
>>> chars is very short for a max length.
>>>
>>> -- Kevin
>>>
>>>
>>> On 7/9/2024 8:22 AM, John Hendrikx wrote:
>>>
>>>
>>> On 09/07/2024 16:52, Andy Goryachev wrote:
>>>
>>>
>>>
>>> Two test files consistently generate an error in Eclipse
>>>
>>> - ObservableValueFluentBindingsTest
>>> - LazyObjectBindingTest
>>>
>>>
>>>
>>> I admit I have a weird setup (EncFS on Linux Mint running on MacBook
>>> Pro), and it only manifests itself in Eclipse and not in the gradle build -
>>> perhaps Eclipse actually verifies the removal of files?
>>>
>>>
>>>
>>> Anyway, a suggestion - if you use @Nested, please keep the class names
>>> *short*.
>>>
>>> This is not an Eclipse bug as I never encounter such issues.  143
>>> characters is rather short these days, but I suppose we could limit the
>>> nesting a bit.  Still, I'd look into a way to alleviate this problem in
>>> your setup, sooner or later this is going to be a problem.
>>>
>>> --John
>>>
>>>
>>>

Re: [External] : Re: consistent naming for tests

[Nir Lisker]

My Eclipse never had this long filename problem, and I reviewed the fluent
bindings PR when it was written so I would have seen it. You can try the
most basic version of Eclipse (
https://download.eclipse.org/eclipse/downloads/) to see if it still happens
if you want to dig into it.

On Tue, Jul 9, 2024 at 9:09 PM Andy Goryachev 
wrote:

> I wonder what other filesystems do?  I just want our code to compile in
> Eclipse on Linux Mint.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *John Hendrikx 
> *Date: *Tuesday, July 9, 2024 at 11:04
> *To: *Andy Goryachev , Johan Vos <
> johan@gluonhq.com>, openjfx-dev 
> *Subject: *Re: [External] : Re: consistent naming for tests
>
> Perhaps, and I guess we're lucky the classes don't fully overlap then...
> if encfs just cuts off too long names when reading/writing, then as long as
> the filename is still unique enough that is going to work.  As soon as two
> file names would overlap, they would overwrite each other and there's no
> way that the code would still work then.
>
> I doubt however this is reasonable to fix in Eclipse; the filesystem is
> not behaving correctly -- encfs should error out instead of silently
> truncating too long names.
>
> --John
>
> On 09/07/2024 19:50, Andy Goryachev wrote:
>
> or gradle may not be verifying that the file is actually deleted.
>
>
>
> Eclipse allows for online replacement (? or whatever that feature is
> called when it can recompile and replace classes in a running vm), so
> perhaps it is more diligent when it comes to deleting.
>
>
>
> -andy
>
>
>
> *From: *John Hendrikx  
> *Date: *Tuesday, July 9, 2024 at 10:47
> *To: *Andy Goryachev 
> , Johan Vos 
> , openjfx-dev 
> 
> *Subject: *Re: [External] : Re: consistent naming for tests
>
> Then I can't explain why it doesn't fail on Gradle; it must be generating
> similar named classes then, but perhaps at a different location (not on
> encfs) ?.
>
> --John
>
> On 09/07/2024 19:35, Andy Goryachev wrote:
>
> Anonymous classes are named $1.  Nested classes retain their name.
>
>
>
> From the ticket:
>
>
>
> https://bugs.openjdk.org/browse/JDK-8334497
>
>
>
> Could not delete:
> /home/ag/Projects/jfx-2/jfx/rt/modules/javafx.base/testbin/test/javafx/beans/value/ObservableValueFluentBindingsTest$When_flatMap_Called$WithNotNullReturns_ObservableValue_Which$WhenObservedForInvalidations$AndWhenUnobserved.class.
>
>
>
> -andy
>
>
>
>
>
> *From: *John Hendrikx  
> *Date: *Tuesday, July 9, 2024 at 10:31
> *To: *Andy Goryachev 
> , Johan Vos 
> , openjfx-dev 
> 
> *Subject: *Re: [External] : Re: consistent naming for tests
>
> Perhaps it is something Eclipse does differently.  Normally nested classed
> are numbered ($1, $2), so perhaps ecj is compiling these with differently
> filenames.
>
> --John
>
> On 09/07/2024 17:37, Andy Goryachev wrote:
>
> Have you tried building in Eclipse on the latest Linux Mint?  Or building
> on an EncFS mount?
>
>
>
> I don't know why Mint decided to use EncFS knowing its issues, and I
> suppose I can try fixing my setup (it's a default Mint installation), but I
> was quite surprised myself and thought that it might be just as easy to fix
> the tests... here is how the fix might look:
>
>
>
> https://github.com/andy-goryachev-oracle/jfx/pull/9
> 
>
>
>
> -andy
>
>
>
> *From: *John Hendrikx  
> *Date: *Tuesday, July 9, 2024 at 08:22
> *To: *Andy Goryachev 
> , Johan Vos 
> , openjfx-dev 
> 
> *Subject: *[External] : Re: consistent naming for tests
>
>
>
> On 09/07/2024 16:52, Andy Goryachev wrote:
>
>
>
> Two test files consistently generate an error in Eclipse
>
> - ObservableValueFluentBindingsTest
> - LazyObjectBindingTest
>
>
>
> I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro),
> and it only manifests itself in Eclipse and not in the gradle build -
> perhaps Eclipse actually verifies the removal of files?
>
>
>
> Anyway, a suggestion - if you use @Nested, please keep the class names
> *short*.
>
> This is not an Eclipse bug as I never encounter such issues.  143
> characters is rather short these days, but I suppose we could limit the
> nesting a bit.  Still, I'd look into a way to alleviate this problem in
> your setup, sooner or later this is going to be a problem.
>
> --John
>
>

Re: consistent naming for tests

[Nir Lisker]

>
> * in some cases, tests are always prefixed with `test` (e.g. `testFoo()`)
> * in some cases, tests have a concise but somehow meaningful name (e.g.
> `testScrollBarStaysVisible`)


Prefixing 'test' was an old convention for testing frameworks. I have been
dropping that prefix in my projects since I'm in a test
class/package/source folder anyway, and it's not like there're methods in a
test class that aren't used for testing. I also use long descriptive names,
like 'newValueNotSetIfOldValueWasInvalid()' or, alternatively,
'doNotSetNewValueIfOldValueWasInvalid()'. John's nesting names are also
good when nesting is appropriate.

* in some cases, tests refer to JBS issues (e.g. testJDK8309935)

* in some cases, the test is explained in comments.


I don't like JBS numbers as names, but I like them as links in a comment. I
prefer the name of the test and methods to be self-explanatory, like in
non-test code, rather than comments. However, sometimes comments are needed
because of tricky or non-trivial situations, which is part of what tests
are for.


On Tue, Jul 9, 2024 at 6:30 PM Kevin Rushforth 
wrote:

> This might be a combination of Eclipse and eCryptfs. I agree that 143
> chars is very short for a max length.
>
> -- Kevin
>
>
> On 7/9/2024 8:22 AM, John Hendrikx wrote:
>
>
> On 09/07/2024 16:52, Andy Goryachev wrote:
>
>
>
> Two test files consistently generate an error in Eclipse
>
> - ObservableValueFluentBindingsTest
> - LazyObjectBindingTest
>
>
>
> I admit I have a weird setup (EncFS on Linux Mint running on MacBook Pro),
> and it only manifests itself in Eclipse and not in the gradle build -
> perhaps Eclipse actually verifies the removal of files?
>
>
>
> Anyway, a suggestion - if you use @Nested, please keep the class names
> *short*.
>
> This is not an Eclipse bug as I never encounter such issues.  143
> characters is rather short these days, but I suppose we could limit the
> nesting a bit.  Still, I'd look into a way to alleviate this problem in
> your setup, sooner or later this is going to be a problem.
>
> --John
>
>
>

Re: RFR: 8335218: Eclipse Config: Remove Gradle Integration

[Nir Lisker]

On Wed, 26 Jun 2024 21:23:34 GMT, Andy Goryachev  wrote:

> This might be controversial. I am proposing to remove the Gradle integration 
> in the Eclipse config files.
> 
> Problem
> ===
> Eclipse Gradle integration (Buildship) cannot import the OpenJFX build.gradle 
> cleanly. Every time the project is imported into a new workspace (or 
> re-opened after being closed) it executes Gradle, creates and modifies a 
> number of Eclipse .project and .classpath files, all of which need to be 
> reverted for Eclipse workspace to become usable again.
> 
> Solution
> ==
> Remove Gradle nature from the Eclipse project files. This change only affects 
> Eclipse config files and does not impact build.gradle or other IDEs.
> 
> Advantages
> =
> 1. The multiple nested projects in the repo will get imported cleanly on the 
> first attempt, will not require additional steps to clear the Buildship 
> changes.
> 2. completely removes the dependency on the Eclipse Buildship and its 
> idiosyncrasies.
> 
> NOTES:
> - even though the reverse was done for IntelliJ, but its gradle import still 
> does not import tests cleanly, see 
> [JDK-8223373](https://bugs.openjdk.org/browse/JDK-8223373)
> - this improvement contradicts 
> [JDK-8223374](https://bugs.openjdk.org/browse/JDK-8223374) as without Eclipse 
> files in the repo, it will be impossible to use Eclipse in a meaningful way 
> without the fully functional Buildship support, and that is a big IF.
> - once integrated, Eclipse users would only need to re-import the main 
> project with 'search for nested projects' enabled

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1491#pullrequestreview-2164597681

Re: RFR: 8335218: Eclipse Config: Remove Gradle Integration

[Nir Lisker]

On Mon, 8 Jul 2024 09:08:19 GMT, John Hendrikx  wrote:

> I work around this by running the test once, then changing its configuration 
> and adding a standard incantation to its command line (`-Djavafx.toolkit`, 
> `-Djava.library.path`, some `--add-modules`) 

The `--add-modules` could be taken care of by the eclipse plugin. The other 2 
are javafx specific.

> fixing the problems in the gradle build so Buildship recognizes it properly. 
> Since I suspect the latter is not an easy task, removing the Gradle nature 
> seems like a good alternative.

Yes, https://github.com/openjdk/jfx/pull/1232 should be part of the effort to 
simplify the gradle build file. It will take several iterations on that file to 
modernize OpenJFX's build.

> so this is a non-standard setup, meaning it is not described in the 
> https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse 
> wiki page.

Yes, it's not described in the wiki because it can't read the gradle file 
properly to begin with.

> Still, I would like to propose we proceed with this change even if it is a 
> temporary one (until the Buildship plug-in gets mature enough to be able to 
> import the gradle configuration cleanly) because it prevents the said 
> Buildship from messing up the project files and breaking the build every time 
> the project(s) are imported or reopened.

I'm fine with this. Just noting that we're not waiting on Buildship to catch 
up, we need our `build.gradle` to catch up :)

-

PR Comment: https://git.openjdk.org/jfx/pull/1491#issuecomment-2215516698

Re: RFR: 8335218: Eclipse Config: Remove Gradle Integration

[Nir Lisker]

On Wed, 26 Jun 2024 21:23:34 GMT, Andy Goryachev  wrote:

> This might be controversial. I am proposing to remove the Gradle integration 
> in the Eclipse config files.
> 
> Problem
> ===
> Eclipse Gradle integration (Buildship) cannot import the OpenJFX build.gradle 
> cleanly. Every time the project is imported into a new workspace (or 
> re-opened after being closed) it executes Gradle, creates and modifies a 
> number of Eclipse .project and .classpath files, all of which need to be 
> reverted for Eclipse workspace to become usable again.
> 
> Solution
> ==
> Remove Gradle nature from the Eclipse project files. This change only affects 
> Eclipse config files and does not impact build.gradle or other IDEs.
> 
> Advantages
> =
> 1. The multiple nested projects in the repo will get imported cleanly on the 
> first attempt, will not require additional steps to clear the Buildship 
> changes.
> 2. completely removes the dependency on the Eclipse Buildship and its 
> idiosyncrasies.
> 
> NOTES:
> - even though the reverse was done for IntelliJ, but its gradle import still 
> does not import tests cleanly, see 
> [JDK-8223373](https://bugs.openjdk.org/browse/JDK-8223373)
> - this improvement contradicts 
> [JDK-8223374](https://bugs.openjdk.org/browse/JDK-8223374) as without Eclipse 
> files in the repo, it will be impossible to use Eclipse in a meaningful way 
> without the fully functional Buildship support, and that is a big IF.
> - once integrated, Eclipse users would only need to re-import the main 
> project with 'search for nested projects' enabled

Continuing the [mailing 
list](https://mail.openjdk.org/pipermail/openjfx-dev/2024-June/047467.html) 
discussion with more technical details.

Gradle tries to configure Eclipse based on its build file. This includes things 
like source folders, compiler settings, dependencies and others. In the cases 
that I have tried in my projects, this works well. One problem that still 
remains with this configuration is that it's not aware of the locations of the 
modules. This causes missing module errors when launching, which require 
`-add-modules` to remedy. The [eclipse 
plugin](https://docs.gradle.org/current/userguide/eclipse_plugin.html) solves 
this by resolving the paths locally for your gradle cache.

This tends to be the preferred way to do things from my experience since you 
don't need to push eclipse-specific files to the repo. The gradle files need to 
be there anyway, so it makes sense to configure eclipse from them and you 
maintain uniformity between the different development environments. Where this 
fails is with OpenJFX projects. The monolithic and old gradle file uses 
conventions from old gradle versions. This causes faulty generation of the 
eclipse files with and without the eclipse plugin. For example, all the source 
folders (`src/main/java`, `src/test/java`...) are prepended with the 
project/module name (`base/src/main/java`) because of manual source set 
configurations that shouldn't exist, at least not the way they are now.

The problem presented in the issue has 2 solutions:
1. Opt out: Include the Gralde nature by default, do all the initial building, 
then revert the Eclipse files. If you want to disable the Gradle integration, 
you can do so as well be removing the nature within the IDE.
2. Opt in: Don't include the Gradle nature by default and do all the initial 
building. If you want the Gradle integration, you can add it within the IDE.
Both of these are one-time actions.

I'm fine with either since I find the one-time action to be cheap.

By the way, with this change you will also have to update the [wiki 
page](https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse)
 to a state where the Java import is the standard and there's an opt-in for the 
Gradle integration.

@hjohn Do you have any remarks?

-

PR Comment: https://git.openjdk.org/jfx/pull/1491#issuecomment-2212068694

Re: Should we document Styleable properties?

[Nir Lisker]

As discussed previously regarding auto-generation of css styleables, an
annotation on the property can be made displayable in javadoc. The
problem is that it will display *all* the parameters of the annotation. For
autogeneration, we need more than what we would like to display, so perhaps
two different annotations could work, one for the autogeneration and one
for doc marking.

On Tue, Jul 2, 2024 at 3:49 PM Eric Bresie  wrote:

> A little off topic but…
>
> There is a pending JEP for allowing markup to be included in javadocs
> https://openjdk.org/jeps/467
>
> Eric Bresie
> ebre...@gmail.com
>
>
> On Mon, Jul 1, 2024 at 4:35 PM Michael Strauß 
> wrote:
>
>> Further testing shows that while the javadoc tool does support custom
>> tags, it doesn’t lift custom tags defined on JavaFX property fields to the
>> property method documentation.
>>
>> Lifting documentation from the property field to property methods is a
>> JavaFX-specific feature of Javadoc. At first glance, there seems to be no
>> particular reason why custom tags are ignored in this situation; probably
>> it just wasn’t implemented.
>>
>> So if we add this custom tag, it won’t show up on the generated HTML. We
>> could start using a custom tag, and then file an enhancement request for
>> Javadoc to lift custom tags to methods just as it does with other tags.
>> What’s your opinion on that?
>>
>>
>> Andy Goryachev  schrieb am Mo. 1. Juli 2024
>> um 23:20:
>>
>>> Would even work with Eclipse out of the box:
>>>
>>>
>>>
>>>
>>>
>>> I also like the fact that we won't need to maintain links manually as
>>> there would not be any.
>>>
>>>
>>>
>>> -andy
>>>
>>

Re: RFR: 8332895: Support interpolation for backgrounds and borders

[Nir Lisker]

On Mon, 3 Jun 2024 09:48:40 GMT, Florian Kirmaier  wrote:

> Note that this PR also changes the specification of Interpolatable to make 
> users aware that they shouldn't assume any particular identity of the object 
> returned from the interpolate() method. This allows the implementation to 
> re-use objects and reduce the number of object allocations.

There is an issue for that already: 
https://bugs.openjdk.org/browse/JDK-8226911. You can take it.

-

PR Comment: https://git.openjdk.org/jfx/pull/1471#issuecomment-2151085981

Re: RFR: 8332895: Support interpolation for backgrounds and borders [v2]

[Nir Lisker]

On Tue, 4 Jun 2024 20:46:28 GMT, Kevin Rushforth  wrote:

> @nlisker would you be willing to be the second reviewer?

I could do some of the review, but probably no time for a full one.

> Since you propose interpolating objects that aren't simple sets of 
> floating-point fields, the overridden interpolate method should specify the 
> behavior of the fields that are not. For example, different types of Paint 
> are sometimes interpolatable and sometimes return either the start or end; 
> some fields of BackgroundImage are interpolatable and others (notably the 
> image itself) return either the start or the end; and so forth.

When I did the (simple) interpolation work on 
[Point2D/3D](https://bugs.openjdk.org/browse/JDK-8226454) I also looked at 
`Border` and `Background` because animating the color of these is rather 
common. The problem is that they are rather complex and don't interpolate 
trivially as a whole. As a user, if I want to interpolate between images, I 
would think of a smooth transition between them, not a jump cut. There are 
other oddities as well.

-

PR Comment: https://git.openjdk.org/jfx/pull/1471#issuecomment-2151084870

Re: [PATCH 3/4] iotests: Change imports for Python 3.13

[Nir Soffer]

> On 2 Jul 2024, at 17:44, John Snow  wrote:
> 
> 
> 
> On Tue, Jul 2, 2024 at 7:52 AM Nir Soffer  <mailto:nsof...@redhat.com>> wrote:
>> On Thu, Jun 27, 2024 at 2:23 AM John Snow > <mailto:js...@redhat.com>> wrote:
>> >
>> > Python 3.13 isn't out yet, but it's in beta and Fedora is ramping up to
>> > make it the default system interpreter for Fedora 41.
>> >
>> > They moved our cheese for where ContextManager lives; add a conditional
>> > to locate it while we support both pre-3.9 and 3.13+.
>> >
>> > Signed-off-by: John Snow mailto:js...@redhat.com>>
>> > ---
>> >  tests/qemu-iotests/testenv.py| 7 ++-
>> >  tests/qemu-iotests/testrunner.py | 9 ++---
>> >  2 files changed, 12 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
>> > index 588f30a4f14..96d69e56963 100644
>> > --- a/tests/qemu-iotests/testenv.py
>> > +++ b/tests/qemu-iotests/testenv.py
>> > @@ -25,7 +25,12 @@
>> >  import random
>> >  import subprocess
>> >  import glob
>> > -from typing import List, Dict, Any, Optional, ContextManager
>> > +from typing import List, Dict, Any, Optional
>> > +
>> > +if sys.version_info >= (3, 9):
>> > +from contextlib import AbstractContextManager as ContextManager
>> > +else:
>> > +from typing import ContextManager
>> 
>> It can be cleaner to add a compat module hiding the details so the
>> entire project
>> can have a single instance of this. Other code will just use:
>> 
>> from compat import ContextManager
> 
> If there were more than two uses, I'd consider it. As it stands, a compat.py 
> module with just one import conditional in it doesn't seem worth the hassle. 
> Are there more cases of compatibility goop inside iotests that need to be 
> factored out to make it worth it?

I don’t about other. For me even one instance is ugly enough :-)

Re: [PATCH 3/4] iotests: Change imports for Python 3.13

[Nir Soffer]

> On 2 Jul 2024, at 17:44, John Snow  wrote:
> 
> 
> 
> On Tue, Jul 2, 2024 at 7:52 AM Nir Soffer  <mailto:nsof...@redhat.com>> wrote:
>> On Thu, Jun 27, 2024 at 2:23 AM John Snow > <mailto:js...@redhat.com>> wrote:
>> >
>> > Python 3.13 isn't out yet, but it's in beta and Fedora is ramping up to
>> > make it the default system interpreter for Fedora 41.
>> >
>> > They moved our cheese for where ContextManager lives; add a conditional
>> > to locate it while we support both pre-3.9 and 3.13+.
>> >
>> > Signed-off-by: John Snow mailto:js...@redhat.com>>
>> > ---
>> >  tests/qemu-iotests/testenv.py| 7 ++-
>> >  tests/qemu-iotests/testrunner.py | 9 ++---
>> >  2 files changed, 12 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
>> > index 588f30a4f14..96d69e56963 100644
>> > --- a/tests/qemu-iotests/testenv.py
>> > +++ b/tests/qemu-iotests/testenv.py
>> > @@ -25,7 +25,12 @@
>> >  import random
>> >  import subprocess
>> >  import glob
>> > -from typing import List, Dict, Any, Optional, ContextManager
>> > +from typing import List, Dict, Any, Optional
>> > +
>> > +if sys.version_info >= (3, 9):
>> > +from contextlib import AbstractContextManager as ContextManager
>> > +else:
>> > +from typing import ContextManager
>> 
>> It can be cleaner to add a compat module hiding the details so the
>> entire project
>> can have a single instance of this. Other code will just use:
>> 
>> from compat import ContextManager
> 
> If there were more than two uses, I'd consider it. As it stands, a compat.py 
> module with just one import conditional in it doesn't seem worth the hassle. 
> Are there more cases of compatibility goop inside iotests that need to be 
> factored out to make it worth it?

I don’t about other. For me even one instance is ugly enough :-)

Re: [PATCH 3/4] iotests: Change imports for Python 3.13

[Nir Soffer]

On Thu, Jun 27, 2024 at 2:23 AM John Snow  wrote:
>
> Python 3.13 isn't out yet, but it's in beta and Fedora is ramping up to
> make it the default system interpreter for Fedora 41.
>
> They moved our cheese for where ContextManager lives; add a conditional
> to locate it while we support both pre-3.9 and 3.13+.
>
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/testenv.py| 7 ++-
>  tests/qemu-iotests/testrunner.py | 9 ++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 588f30a4f14..96d69e56963 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -25,7 +25,12 @@
>  import random
>  import subprocess
>  import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional
> +
> +if sys.version_info >= (3, 9):
> +from contextlib import AbstractContextManager as ContextManager
> +else:
> +from typing import ContextManager

It can be cleaner to add a compat module hiding the details so the
entire project
can have a single instance of this. Other code will just use:

from compat import ContextManager

>
>  DEF_GDB_OPTIONS = 'localhost:12345'
>
> diff --git a/tests/qemu-iotests/testrunner.py 
> b/tests/qemu-iotests/testrunner.py
> index 7b322272e92..2e236c8fa39 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -27,11 +27,14 @@
>  import shutil
>  import sys
>  from multiprocessing import Pool
> -from typing import List, Optional, Any, Sequence, Dict, \
> -ContextManager
> -
> +from typing import List, Optional, Any, Sequence, Dict
>  from testenv import TestEnv
>
> +if sys.version_info >= (3, 9):
> +from contextlib import AbstractContextManager as ContextManager
> +else:
> +from typing import ContextManager
> +
>
>  def silent_unlink(path: Path) -> None:
>  try:
> --
> 2.45.0
>
>

Re: [PATCH 3/4] iotests: Change imports for Python 3.13

[Nir Soffer]

On Thu, Jun 27, 2024 at 2:23 AM John Snow  wrote:
>
> Python 3.13 isn't out yet, but it's in beta and Fedora is ramping up to
> make it the default system interpreter for Fedora 41.
>
> They moved our cheese for where ContextManager lives; add a conditional
> to locate it while we support both pre-3.9 and 3.13+.
>
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/testenv.py| 7 ++-
>  tests/qemu-iotests/testrunner.py | 9 ++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 588f30a4f14..96d69e56963 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -25,7 +25,12 @@
>  import random
>  import subprocess
>  import glob
> -from typing import List, Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional
> +
> +if sys.version_info >= (3, 9):
> +from contextlib import AbstractContextManager as ContextManager
> +else:
> +from typing import ContextManager

It can be cleaner to add a compat module hiding the details so the
entire project
can have a single instance of this. Other code will just use:

from compat import ContextManager

>
>  DEF_GDB_OPTIONS = 'localhost:12345'
>
> diff --git a/tests/qemu-iotests/testrunner.py 
> b/tests/qemu-iotests/testrunner.py
> index 7b322272e92..2e236c8fa39 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -27,11 +27,14 @@
>  import shutil
>  import sys
>  from multiprocessing import Pool
> -from typing import List, Optional, Any, Sequence, Dict, \
> -ContextManager
> -
> +from typing import List, Optional, Any, Sequence, Dict
>  from testenv import TestEnv
>
> +if sys.version_info >= (3, 9):
> +from contextlib import AbstractContextManager as ContextManager
> +else:
> +from typing import ContextManager
> +
>
>  def silent_unlink(path: Path) -> None:
>  try:
> --
> 2.45.0
>
>

Strange behavior of qemu-img map: zero/data status depends on fallocated image page cache content

[Nir Soffer]

ng storage may not  be  reported
   as  a  hole.)  In the simplest implementation, a filesystem can support
   the operations by making SEEK_HOLE always return the offset of the  end
   of  the  file, and making SEEK_DATA always return offset (i.e., even if
   the location referred to by offset is a hole, it can be  considered  to
   consist of data that is a sequence of zeros).

On xfs filesystem we can inspect the actual allocation:

$ xfs_bmap -v falloc.img
falloc.img:
 EXT: FILE-OFFSET  BLOCK-RANGE  AG AG-OFFSET      TOTAL
   0: [0..7]:  192..199  0 (192..199) 8
   1: [8..2097151]:200..2097343  0 (200..2097343)   2097144

$ xfs_bmap -v sparse.img
sparse.img:
 EXT: FILE-OFFSET  BLOCK-RANGE  AG AG-OFFSETTOTAL
   0: [0..7]:  2097344..2097351  0 (2097344..2097351)   8
   1: [8..2047]:   2097352..2099391  0 (2097352..2099391)2040
   2: [2048..2097151]: hole   2095104

Maybe qemu-img should use file system specific APIs like ioctl_xfs_getbmap(2)
to get more correct and consistent allocation info?


Nir

Re: How to stop QEMU punching holes in backing store

[Nir Soffer]

On Wed, Jun 19, 2024 at 2:18 PM Nir Soffer  wrote:
> On 19 Jun 2024, at 8:54, Justin  wrote:
>
>I've run strace and I see calls to fallocate with these flags:
>FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE
>
>I've tried passing these options: discard=off,detect-zeroes=off but
>this does not help. This is the full set of relevant options I'm
>using:
>
>-drive file=/vms/vm0/drive,format=raw,if=virtio,discard=off,detect-zeroes=
>off
>
> You don't need to disable detect-zeros - in my tests it makes dd if=/dev/zero
> 5 times faster (770 MiB/s -> 3 GiB/s) since zero writes are converted to
> fallocate(FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE).
>
> The issue seems to be ignoring the discard option when opening the image,
> and is fixed by this:
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
>
> Thanks. When might this patch (or something similar) be merged?
>
> The patch need more work, when the work is done and qemu maintainers are 
> happy it is a good estimate :-)

Justin, v3 should be ready now, do you want to help by testing it?

v3:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00644.html

You can also pull it from:
https://gitlab.com/nirs/qemu/-/tree/consider-discard-option

Nir

backup-discard-source iotest fails on xfs, pass on ext4

[Nir Soffer]

While testing 
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00644.html
I noticed that
this tests is failing also with master
(28b8a57ad63670aa0ce90334523dc552b13b4336).

The test succeeds with ext4, and fail with xfs.

Nir

---

Success on ext4:

$ TEST_DIR=/data/tetests/qemu-iotests/check backup-discard-source -qcow2
test-ext4/ test-xfs/
[nsoffer build (master)]$ TEST_DIR=/data/test-ext4/scratch
tests/qemu-iotests/check backup-discard-source -qcow2
QEMU  -- "/home/nsoffer/src/qemu/build/qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG  -- "/home/nsoffer/src/qemu/build/qemu-img"
QEMU_IO   -- "/home/nsoffer/src/qemu/build/qemu-io" --cache
writeback --aio threads -f qcow2
QEMU_NBD  -- "/home/nsoffer/src/qemu/build/qemu-nbd"
IMGFMT-- qcow2
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 sparse.local 6.9.4-100.fc39.x86_64
TEST_DIR  -- /data/test-ext4/scratch
SOCK_DIR  -- /tmp/qemu-iotests-rdoofjdc
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

backup-discard-source   pass   [00:14:09] [00:14:10]   0.8s
Passed all 1 iotests


Failing on xfs:

$ TEST_DIR=/data/test-xfs/scratch tests/qemu-iotests/check
backup-discard-source -qcow2
QEMU  -- "/home/nsoffer/src/qemu/build/qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG  -- "/home/nsoffer/src/qemu/build/qemu-img"
QEMU_IO   -- "/home/nsoffer/src/qemu/build/qemu-io" --cache
writeback --aio threads -f qcow2
QEMU_NBD  -- "/home/nsoffer/src/qemu/build/qemu-nbd"
IMGFMT-- qcow2
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 sparse.local 6.9.4-100.fc39.x86_64
TEST_DIR  -- /data/test-xfs/scratch
SOCK_DIR  -- /tmp/qemu-iotests-4j_gu404
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

backup-discard-source   fail   [00:15:44] [00:15:45]   0.8s
(last: 0.8s)  failed, exit status 1
--- /home/nsoffer/src/qemu/tests/qemu-iotests/tests/backup-discard-source.out
+++ 
/data/test-xfs/scratch/qcow2-file-backup-discard-source/backup-discard-source.out.bad
@@ -1,5 +1,14 @@
-..
+F.
+==
+FAIL: test_discard_cbw (__main__.TestBackup.test_discard_cbw)
+1. do backup(discard_source=True), which should inform
+--
+Traceback (most recent call last):
+  File "/home/nsoffer/src/qemu/tests/qemu-iotests/tests/backup-discard-source",
line 147, in test_discard_cbw
+self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+AssertionError: 1249280 not less than 524288
+
 --
 Ran 2 tests

-OK
+FAILED (failures=1)
Failures: backup-discard-source
Failed 1 of 1 iotests


$ xfs_info /data/test-xfs/
meta-data=/dev/mapper/data-test--xfs isize=512agcount=4, agsize=655360 blks
 =   sectsz=512   attr=2, projid32bit=1
 =   crc=1finobt=1, sparse=1, rmapbt=0
 =   reflink=1bigtime=1 inobtcount=1 nrext64=0
data =   bsize=4096   blocks=2621440, imaxpct=25
 =   sunit=0  swidth=0 blks
naming   =version 2  bsize=4096   ascii-ci=0, ftype=1
log  =internal log   bsize=4096   blocks=16384, version=2
 =   sectsz=512   sunit=0 blks, lazy-count=1
realtime =none   extsz=4096   blocks=0, rtextents=0

[PATCH v3 0/2] Consider discard option when writing zeros

[Nir Soffer]

Punch holes only when the image is opened with discard=on or discard=unmap.

Tested by:
- new write-zeroes-unmap iotest on xfs, ext4, and tmpfs
- tests/qemu-iotests/check -raw
- tests/qemu-iotests/check -qcow2

Changes since v2
- Add write-zeroes-unmap iotest
- Fix iotest missing discard=unmap

v2 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00231.html

Nir Soffer (2):
  qemu-iotest/245: Add missing discard=unmap
  Consider discard option when writing zeros

 block/io.c|   9 +-
 tests/qemu-iotests/245|   2 +-
 tests/qemu-iotests/tests/write-zeroes-unmap   | 127 ++
 .../qemu-iotests/tests/write-zeroes-unmap.out |  81 +++
 4 files changed, 214 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/write-zeroes-unmap
 create mode 100644 tests/qemu-iotests/tests/write-zeroes-unmap.out

-- 
2.45.2

[PATCH v3 1/2] qemu-iotest/245: Add missing discard=unmap

[Nir Soffer]

The test works since we punch holes by default even when opening the
image without discard=on or discard=unmap. Fix the test to enable
discard.
---
 tests/qemu-iotests/245 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a934c9d1e6..f96610f510 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -590,11 +590,11 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
 # Insert (and remove) a compress filter
 @iotests.skip_if_unsupported(['compress'])
 def test_insert_compress_filter(self):
 # Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file)
-opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)}
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0), 
'discard': 'unmap'}
 self.vm.cmd('blockdev-add', conv_keys = False, **opts)
 
 # Add a 'compress' filter
 filter_opts = {'driver': 'compress',
'node-name': 'compress0',
-- 
2.45.2

[PATCH v3 1/2] qemu-iotest/245: Add missing discard=unmap

[Nir Soffer]

The test works since we punch holes by default even when opening the
image without discard=on or discard=unmap. Fix the test to enable
discard.
---
 tests/qemu-iotests/245 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index a934c9d1e6..f96610f510 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -590,11 +590,11 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 
 # Insert (and remove) a compress filter
 @iotests.skip_if_unsupported(['compress'])
 def test_insert_compress_filter(self):
 # Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file)
-opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)}
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0), 
'discard': 'unmap'}
 self.vm.cmd('blockdev-add', conv_keys = False, **opts)
 
 # Add a 'compress' filter
 filter_opts = {'driver': 'compress',
'node-name': 'compress0',
-- 
2.45.2

[PATCH v3 2/2] Consider discard option when writing zeros

[Nir Soffer]

When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images[1].

bdrv_co_pwrite_zeroes() correctly disables BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.

This change implements the documented behavior, punching holes only when
opening the image with discard=on or discard=unmap. This may not be the
best default but can improve it later.

The test depends on a file system supporting discard, deallocating the
entire file when punching hole with the length of the entire file.
Tested with xfs, ext4, and tmpfs.

[1] https://lists.nongnu.org/archive/html/qemu-discuss/2024-06/msg3.html

Signed-off-by: Nir Soffer 
---
 block/io.c|   9 +-
 tests/qemu-iotests/tests/write-zeroes-unmap   | 127 ++
 .../qemu-iotests/tests/write-zeroes-unmap.out |  81 +++
 3 files changed, 213 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/write-zeroes-unmap
 create mode 100644 tests/qemu-iotests/tests/write-zeroes-unmap.out

diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t 
offset, int64_t bytes,
 /* By definition there is no user buffer so this flag doesn't make sense */
 if (flags & BDRV_REQ_REGISTERED_BUF) {
 return -EINVAL;
 }
 
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
 /* Invalidate the cached block-status data range if this write overlaps */
 bdrv_bsc_invalidate_range(bs, offset, bytes);
 
 assert(alignment % bs->bl.request_alignment == 0);
 head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
*child, int64_t offset,
 {
 IO_CODE();
 trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 assert_bdrv_graph_readable();
 
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
 return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
diff --git a/tests/qemu-iotests/tests/write-zeroes-unmap 
b/tests/qemu-iotests/tests/write-zeroes-unmap
new file mode 100755
index 00..7cfeeaf839
--- /dev/null
+++ b/tests/qemu-iotests/tests/write-zeroes-unmap
@@ -0,0 +1,127 @@
+#!/usr/bin/env bash
+# group: quick
+#
+# Test write zeros unmap.
+#
+# Copyright (C) Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+trap _cleanup_test_img exit
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+create_test_image() {
+_make_test_img -f $IMGFMT 1m
+}
+
+filter_command() {
+_filter_testdir | _filter_qemu_io | _filter_qemu | _filter_hmp
+}
+
+print_disk_usage() {
+du -sh $TEST_IMG | _filter_testdir
+}
+
+echo
+echo "=== defaults - write zeros ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -z 0 1m"\nquit' \
+| $QEMU -monitor stdio -drive if=none,file=$TEST_IMG,format=$IMGFMT \
+| filter_command
+print_disk_usage
+
+echo
+echo "=== defaults - write zeros unmap ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' \
+| $QEMU -monitor stdio -drive if=none,file=$TEST_IMG,format=$IMGFMT \
+| filter_command
+print_disk_usage
+
+
+echo
+echo "=== defaults - write actual zeros ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' \
+| $QEMU -monitor stdio -drive if=none,file=$TEST_IMG,format=$IMGFMT \
+| filter_command
+print_disk_usage
+
+echo
+echo "=== discard=off - write zeroes unmap ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -zu 0 1m&q

[PATCH v3 2/2] Consider discard option when writing zeros

[Nir Soffer]

When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images[1].

bdrv_co_pwrite_zeroes() correctly disables BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.

This change implements the documented behavior, punching holes only when
opening the image with discard=on or discard=unmap. This may not be the
best default but can improve it later.

The test depends on a file system supporting discard, deallocating the
entire file when punching hole with the length of the entire file.
Tested with xfs, ext4, and tmpfs.

[1] https://lists.nongnu.org/archive/html/qemu-discuss/2024-06/msg3.html

Signed-off-by: Nir Soffer 
---
 block/io.c|   9 +-
 tests/qemu-iotests/tests/write-zeroes-unmap   | 127 ++
 .../qemu-iotests/tests/write-zeroes-unmap.out |  81 +++
 3 files changed, 213 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/write-zeroes-unmap
 create mode 100644 tests/qemu-iotests/tests/write-zeroes-unmap.out

diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t 
offset, int64_t bytes,
 /* By definition there is no user buffer so this flag doesn't make sense */
 if (flags & BDRV_REQ_REGISTERED_BUF) {
 return -EINVAL;
 }
 
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
 /* Invalidate the cached block-status data range if this write overlaps */
 bdrv_bsc_invalidate_range(bs, offset, bytes);
 
 assert(alignment % bs->bl.request_alignment == 0);
 head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
*child, int64_t offset,
 {
 IO_CODE();
 trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 assert_bdrv_graph_readable();
 
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
 return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
diff --git a/tests/qemu-iotests/tests/write-zeroes-unmap 
b/tests/qemu-iotests/tests/write-zeroes-unmap
new file mode 100755
index 00..7cfeeaf839
--- /dev/null
+++ b/tests/qemu-iotests/tests/write-zeroes-unmap
@@ -0,0 +1,127 @@
+#!/usr/bin/env bash
+# group: quick
+#
+# Test write zeros unmap.
+#
+# Copyright (C) Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+trap _cleanup_test_img exit
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+create_test_image() {
+_make_test_img -f $IMGFMT 1m
+}
+
+filter_command() {
+_filter_testdir | _filter_qemu_io | _filter_qemu | _filter_hmp
+}
+
+print_disk_usage() {
+du -sh $TEST_IMG | _filter_testdir
+}
+
+echo
+echo "=== defaults - write zeros ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -z 0 1m"\nquit' \
+| $QEMU -monitor stdio -drive if=none,file=$TEST_IMG,format=$IMGFMT \
+| filter_command
+print_disk_usage
+
+echo
+echo "=== defaults - write zeros unmap ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' \
+| $QEMU -monitor stdio -drive if=none,file=$TEST_IMG,format=$IMGFMT \
+| filter_command
+print_disk_usage
+
+
+echo
+echo "=== defaults - write actual zeros ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' \
+| $QEMU -monitor stdio -drive if=none,file=$TEST_IMG,format=$IMGFMT \
+| filter_command
+print_disk_usage
+
+echo
+echo "=== discard=off - write zeroes unmap ==="
+echo
+
+create_test_image
+echo -e 'qemu-io none0 "write -zu 0 1m&q

[PATCH v3 0/2] Consider discard option when writing zeros

[Nir Soffer]

Punch holes only when the image is opened with discard=on or discard=unmap.

Tested by:
- new write-zeroes-unmap iotest on xfs, ext4, and tmpfs
- tests/qemu-iotests/check -raw
- tests/qemu-iotests/check -qcow2

Changes since v2
- Add write-zeroes-unmap iotest
- Fix iotest missing discard=unmap

v2 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00231.html

Nir Soffer (2):
  qemu-iotest/245: Add missing discard=unmap
  Consider discard option when writing zeros

 block/io.c|   9 +-
 tests/qemu-iotests/245|   2 +-
 tests/qemu-iotests/tests/write-zeroes-unmap   | 127 ++
 .../qemu-iotests/tests/write-zeroes-unmap.out |  81 +++
 4 files changed, 214 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/write-zeroes-unmap
 create mode 100644 tests/qemu-iotests/tests/write-zeroes-unmap.out

-- 
2.45.2

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Thu, Jun 27, 2024 at 2:42 PM Kevin Wolf  wrote:

> Am 26.06.2024 um 18:27 hat Nir Soffer geschrieben:
> > On Wed, Jun 26, 2024 at 12:17 PM Daniel P. Berrangé  >
> > wrote:
> >
> > > On Mon, Jun 24, 2024 at 06:08:26PM +0200, Kevin Wolf wrote:
> > > > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > > > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > > > > Tested using:
> > > > >
> > > > > Hi Nir,
> > > > > This looks like a good candidate for the qemu-iotests test suite.
> > > Adding
> > > > > it to the automated tests will protect against future regressions.
> > > > >
> > > > > Please add the script and the expected output to
> > > > > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > > > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > > > >
> > > > > See the existing test cases in tests/qemu-iotests/ and
> > > > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > > > > others are Python. I think shell makes sense for this test case.
> You
> > > > > can copy the test framework boilerplate from an existing test case.
> > > >
> > > > 'du' can't be used like this in qemu-iotests because it makes
> > > > assumptions that depend on the filesystem. A test case replicating
> what
> > > > Nir did manually would likely fail on XFS with its preallocation.
> > > >
> > > > Maybe we could operate on a file exposed by the FUSE export that is
> > > > backed by qcow2, and then you can use 'qemu-img map' on that qcow2
> image
> > > > to verify the allocation status. Somewhat complicated, but I think it
> > > > could work.
> > >
> > > A simpler option would be to use 'du' but with a fuzzy range test,
> > > rather than an exact equality test.
> > >
> > > For the tests which write 1 MB, check the 'du' usage is "at least 1MB",
> > > for the tests which expect to unmap blocks, check that the 'du' usage
> > > is "less than 256kb". This should be within bounds of xfs speculative
> > > allocation.
> >
> > This should work, I'll start with this approach.
>
> If we're okay with accepting tests that depend on filesystem behaviour,
> then 'qemu-img map -f raw --output=json' should be the less risky
> approach than checking 'du'.
>

Unfortunately it does not work since qemu-img map and qemu-nbd reports the
allocated
area as zero area with no data.

I tried this:

$ cat test-print-allocation.sh
#!/bin/sh

qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/qemu-test-unmap.img

echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null

echo "du:"
du -sh $img
echo "qemu-img map:"
qemu-img map -f raw --output json $img
echo "nbdinfo --map:"
nbdinfo --map -- [ qemu-nbd -r -f raw $img ]

echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null

echo "du:"
du -sh $img
echo "qemu-img map:"
qemu-img map -f raw --output json $img
echo "nbdinfo --map:"
nbdinfo --map -- [ qemu-nbd -r -f raw $img ]

rm $img


$ ./test-print-allocation.sh ./qemu-system-x86_64

discard=unmap - write zeroes
du:
1.0M /tmp/qemu-test-unmap.img
qemu-img map:
[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero":
true, "data": false, "offset": 0}]
nbdinfo --map:
 0 10485763  hole,zero

discard=unmap - write zeroes unmap
du:
0 /tmp/qemu-test-unmap.img
qemu-img map:
[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero":
true, "data": false, "offset": 0}]
nbdinfo --map:
 0 10485763  hole,zero

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Thu, Jun 27, 2024 at 2:42 PM Kevin Wolf  wrote:

> Am 26.06.2024 um 18:27 hat Nir Soffer geschrieben:
> > On Wed, Jun 26, 2024 at 12:17 PM Daniel P. Berrangé  >
> > wrote:
> >
> > > On Mon, Jun 24, 2024 at 06:08:26PM +0200, Kevin Wolf wrote:
> > > > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > > > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > > > > Tested using:
> > > > >
> > > > > Hi Nir,
> > > > > This looks like a good candidate for the qemu-iotests test suite.
> > > Adding
> > > > > it to the automated tests will protect against future regressions.
> > > > >
> > > > > Please add the script and the expected output to
> > > > > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > > > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > > > >
> > > > > See the existing test cases in tests/qemu-iotests/ and
> > > > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > > > > others are Python. I think shell makes sense for this test case.
> You
> > > > > can copy the test framework boilerplate from an existing test case.
> > > >
> > > > 'du' can't be used like this in qemu-iotests because it makes
> > > > assumptions that depend on the filesystem. A test case replicating
> what
> > > > Nir did manually would likely fail on XFS with its preallocation.
> > > >
> > > > Maybe we could operate on a file exposed by the FUSE export that is
> > > > backed by qcow2, and then you can use 'qemu-img map' on that qcow2
> image
> > > > to verify the allocation status. Somewhat complicated, but I think it
> > > > could work.
> > >
> > > A simpler option would be to use 'du' but with a fuzzy range test,
> > > rather than an exact equality test.
> > >
> > > For the tests which write 1 MB, check the 'du' usage is "at least 1MB",
> > > for the tests which expect to unmap blocks, check that the 'du' usage
> > > is "less than 256kb". This should be within bounds of xfs speculative
> > > allocation.
> >
> > This should work, I'll start with this approach.
>
> If we're okay with accepting tests that depend on filesystem behaviour,
> then 'qemu-img map -f raw --output=json' should be the less risky
> approach than checking 'du'.
>

Unfortunately it does not work since qemu-img map and qemu-nbd reports the
allocated
area as zero area with no data.

I tried this:

$ cat test-print-allocation.sh
#!/bin/sh

qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/qemu-test-unmap.img

echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null

echo "du:"
du -sh $img
echo "qemu-img map:"
qemu-img map -f raw --output json $img
echo "nbdinfo --map:"
nbdinfo --map -- [ qemu-nbd -r -f raw $img ]

echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null

echo "du:"
du -sh $img
echo "qemu-img map:"
qemu-img map -f raw --output json $img
echo "nbdinfo --map:"
nbdinfo --map -- [ qemu-nbd -r -f raw $img ]

rm $img


$ ./test-print-allocation.sh ./qemu-system-x86_64

discard=unmap - write zeroes
du:
1.0M /tmp/qemu-test-unmap.img
qemu-img map:
[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero":
true, "data": false, "offset": 0}]
nbdinfo --map:
 0 10485763  hole,zero

discard=unmap - write zeroes unmap
du:
0 /tmp/qemu-test-unmap.img
qemu-img map:
[{ "start": 0, "length": 1048576, "depth": 0, "present": true, "zero":
true, "data": false, "offset": 0}]
nbdinfo --map:
 0 10485763  hole,zero

Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs

[Nir Lisker]

On Tue, 4 Apr 2023 15:22:48 GMT, John Hendrikx  wrote:

> This provides and uses a new implementation of `ExpressionHelper`, called 
> `ListenerManager` with improved semantics.
> 
> # Behavior
> 
> |Listener...|ExpressionHelper|ListenerManager|
> |---|---|---|
> |Invocation Order|In order they were registered, invalidation listeners 
> always before change listeners|(unchanged)|
> |Removal during Notification|All listeners present when notification started 
> are notified, but excluded for any nested changes|Listeners are removed 
> immediately regardless of nesting|
> |Addition during Notification|Only listeners present when notification 
> started are notified, but included for any nested changes|New listeners are 
> never called during the current notification regardless of nesting|
> 
> ## Nested notifications:
> 
> | |ExpressionHelper|ListenerManager|
> |---|---|---|
> |Type|Depth first (call stack increases for each nested level)|(same)|
> |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested 
> changes, skipping non-changes|
> |Vetoing Possible?|No|Yes|
> |Old Value correctness|Only for listeners called before listeners making 
> nested changes|Always|
> 
> # Performance
> 
> |Listener|ExpressionHelper|ListenerManager|
> |---|---|---|
> |Addition|Array based, append in empty slot, resize as needed|(same)|
> |Removal|Array based, shift array, resize as needed|(same)|
> |Addition during notification|Array is copied, removing collected 
> WeakListeners in the process|Appended when notification finishes|
> |Removal during notification|As above|Entry is `null`ed (to avoid moving 
> elements in array that is being iterated)|
> |Notification completion with changes|-|Null entries (and collected 
> WeakListeners) are removed|
> |Notifying Invalidation Listeners|1 ns each|(same)|
> |Notifying Change Listeners|1 ns each (*)|2-3 ns each|
> 
> (*) a simple for loop is close to optimal, but unfortunately does not provide 
> correct old values
> 
> # Memory Use 
> 
> Does not include alignment, and assumes a 32-bit VM or one that is using 
> compressed oops.
> 
> |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager|
> |---|---|---|---|
> |No Listeners|none|none|none|
> |Single InvalidationListener|16 bytes overhead|none|none|
> |Single ChangeListener|20 bytes overhead|none|16 bytes overhead|
> |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per 
> listener (excluding unused slots)|61 + 4 per listener (excluding unused 
> slots)|
> 
> # About nested changes
> 
> Nested changes are simply changes that are made to a property that is 
> currently in the process of notifying its listeners. This...

John and I discussed this off-list. I will write a short review of this change.

### Behavior
The solution implements has the following behaviors, which are compared to the 
current (partially-flawed) ones:
* Listeners are invoked in the order they were registered, with invalidation 
listeners being invoked before change listeners. This is also the current 
behavior. The behavior of invoking all listeners according to the order of 
registrations will be investigated.
* Listeners that are removed during event propagation will be removed 
immediately, and will not receive the event (if they hadn't already). This 
differs from the current behavior of removing the listeners only after the 
event finished (buggy implementation).
* Listeners that are added during event propagation will be effectively added 
after the event finishes, and will not receive the event during which they were 
added. This is also the current behavior (buggy implementation). The behavior 
of adding the listeners immediately, as is done with removal, will be 
investigated.
* Nested events are invoked "depth-first", meaning that the parent event 
propagation is halted until the nested event finishes (see below). This differs 
from the current behavior that takes the "breadth-first" approach - each event 
finishes before the nested one starts (buggy implementation).
* Nested events are only handled by listeners who received the parent event 
already so that they can react to the new change. Listeners that did not 
receive the parent event will only get a single (updated) event so that they 
don't react to irrelevant values. This allows vetoing, This differs from the 
current behavior that sends all events to all listeners (buggy implementation).

### Examples
Suppose 5 change listeners are registered when an event starts.
**Removal during an event**

L1 gets the event
L2 gets the event and removes L4
L3 gets the event and removes L2 (L2 already got the event)
L4 does not get the event (removed by L2)
L5 gets the event
final listeners: L1, L3, L5

**Addition during an event**

L1 gets the event
L2 gets the event and adds L6
L3-L5 get the event
L6 does not get the event (added by L2)
final listeners: L1 - L6

**Nested event** (value change during an event)

The observable value changes from 

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 26, 2024 at 12:17 PM Daniel P. Berrangé 
wrote:

> On Mon, Jun 24, 2024 at 06:08:26PM +0200, Kevin Wolf wrote:
> > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > > Tested using:
> > >
> > > Hi Nir,
> > > This looks like a good candidate for the qemu-iotests test suite.
> Adding
> > > it to the automated tests will protect against future regressions.
> > >
> > > Please add the script and the expected output to
> > > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > >
> > > See the existing test cases in tests/qemu-iotests/ and
> > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > > others are Python. I think shell makes sense for this test case. You
> > > can copy the test framework boilerplate from an existing test case.
> >
> > 'du' can't be used like this in qemu-iotests because it makes
> > assumptions that depend on the filesystem. A test case replicating what
> > Nir did manually would likely fail on XFS with its preallocation.
> >
> > Maybe we could operate on a file exposed by the FUSE export that is
> > backed by qcow2, and then you can use 'qemu-img map' on that qcow2 image
> > to verify the allocation status. Somewhat complicated, but I think it
> > could work.
>
> A simpler option would be to use 'du' but with a fuzzy range test,
> rather than an exact equality test.
>
> For the tests which write 1 MB, check the 'du' usage is "at least 1MB",
> for the tests which expect to unmap blocks, check that the 'du' usage
> is "less than 256kb". This should be within bounds of xfs speculative
> allocation.
>

This should work, I'll start with this approach.

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 26, 2024 at 12:17 PM Daniel P. Berrangé 
wrote:

> On Mon, Jun 24, 2024 at 06:08:26PM +0200, Kevin Wolf wrote:
> > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > > Tested using:
> > >
> > > Hi Nir,
> > > This looks like a good candidate for the qemu-iotests test suite.
> Adding
> > > it to the automated tests will protect against future regressions.
> > >
> > > Please add the script and the expected output to
> > > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > >
> > > See the existing test cases in tests/qemu-iotests/ and
> > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > > others are Python. I think shell makes sense for this test case. You
> > > can copy the test framework boilerplate from an existing test case.
> >
> > 'du' can't be used like this in qemu-iotests because it makes
> > assumptions that depend on the filesystem. A test case replicating what
> > Nir did manually would likely fail on XFS with its preallocation.
> >
> > Maybe we could operate on a file exposed by the FUSE export that is
> > backed by qcow2, and then you can use 'qemu-img map' on that qcow2 image
> > to verify the allocation status. Somewhat complicated, but I think it
> > could work.
>
> A simpler option would be to use 'du' but with a fuzzy range test,
> rather than an exact equality test.
>
> For the tests which write 1 MB, check the 'du' usage is "at least 1MB",
> for the tests which expect to unmap blocks, check that the 'du' usage
> is "less than 256kb". This should be within bounds of xfs speculative
> allocation.
>

This should work, I'll start with this approach.

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 26, 2024 at 11:42 AM Kevin Wolf  wrote:

> Am 24.06.2024 um 23:12 hat Nir Soffer geschrieben:
> > On Mon, Jun 24, 2024 at 7:08 PM Kevin Wolf  wrote:
> >
> > > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > > > Tested using:
> > > >
> > > > Hi Nir,
> > > > This looks like a good candidate for the qemu-iotests test suite.
> Adding
> > > > it to the automated tests will protect against future regressions.
> > > >
> > > > Please add the script and the expected output to
> > > > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > > >
> > > > See the existing test cases in tests/qemu-iotests/ and
> > > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > > > others are Python. I think shell makes sense for this test case. You
> > > > can copy the test framework boilerplate from an existing test case.
> > >
> > > 'du' can't be used like this in qemu-iotests because it makes
> > > assumptions that depend on the filesystem. A test case replicating what
> > > Nir did manually would likely fail on XFS with its preallocation.
> >
> > This is why I did not try to add a new qemu-iotest yet.
> >
> > > Maybe we could operate on a file exposed by the FUSE export that is
> > > backed by qcow2, and then you can use 'qemu-img map' on that qcow2
> image
> > > to verify the allocation status. Somewhat complicated, but I think it
> > > could work.
> >
> > Do we have examples of using the FUSE export? It sounds complicated but
> > being able to test on any file system is awesome. The complexity can be
> > hidden behind simple test helpers.
>
> We seem to have a few tests that use it, and then the fuse protocol
> implementation, too. 308 and file-io-error look relevant.
>
> > Another option is to use a specific file system created for the tests,
> > for example on a loop device. We used userstorage[1] in ovirt to test
> > on specific file systems with known sector size.
>
> Creating loop devices requires root privileges. If I understand
> correctly, userstorage solved that by having a setup phase as root
> before running the tests as a normal user? We don't really have that in
> qemu-iotests.
>
> Some tests require passwordless sudo and are skipped otherwise, but this
> means that in practice they are almost always skipped.
>

Yes, this is the assumption the storage is being created before running the
tests,
for example when setting up a development or CI environment, and the tests
can run with unprivileged user.

> But more important, are you ok with the change?
> >
> > I'm not sure about not creating sparse images by default - this is not
> > consistent with qemu-img convert and qemu-nbd, which do sparsify by
> > default. The old behavior seems better.
>
> Well, your patches make it do what we always claimed it would do, so
> that consistency is certainly a good thing. Unmapping on write_zeroes
> and ignoring truncate is a weird combination anyway that doesn't really
> make any sense to me, so I don't think it's worth preserving. The other
> way around could have been more defensible, but that's not how our bug
> works.
>
> Now, if ignoring all discard requests is a good default these days is a
> separate question and I'm not sure really. Maybe discard=unmap should
> be the default (and apply to both discard are write_zeroes, of course).
>

OK, lets limit the scope to fix the code to match the current docs. We can
tweak
the defaults later.

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 26, 2024 at 11:42 AM Kevin Wolf  wrote:

> Am 24.06.2024 um 23:12 hat Nir Soffer geschrieben:
> > On Mon, Jun 24, 2024 at 7:08 PM Kevin Wolf  wrote:
> >
> > > Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > > > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > > > Tested using:
> > > >
> > > > Hi Nir,
> > > > This looks like a good candidate for the qemu-iotests test suite.
> Adding
> > > > it to the automated tests will protect against future regressions.
> > > >
> > > > Please add the script and the expected output to
> > > > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > > > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> > > >
> > > > See the existing test cases in tests/qemu-iotests/ and
> > > > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > > > others are Python. I think shell makes sense for this test case. You
> > > > can copy the test framework boilerplate from an existing test case.
> > >
> > > 'du' can't be used like this in qemu-iotests because it makes
> > > assumptions that depend on the filesystem. A test case replicating what
> > > Nir did manually would likely fail on XFS with its preallocation.
> >
> > This is why I did not try to add a new qemu-iotest yet.
> >
> > > Maybe we could operate on a file exposed by the FUSE export that is
> > > backed by qcow2, and then you can use 'qemu-img map' on that qcow2
> image
> > > to verify the allocation status. Somewhat complicated, but I think it
> > > could work.
> >
> > Do we have examples of using the FUSE export? It sounds complicated but
> > being able to test on any file system is awesome. The complexity can be
> > hidden behind simple test helpers.
>
> We seem to have a few tests that use it, and then the fuse protocol
> implementation, too. 308 and file-io-error look relevant.
>
> > Another option is to use a specific file system created for the tests,
> > for example on a loop device. We used userstorage[1] in ovirt to test
> > on specific file systems with known sector size.
>
> Creating loop devices requires root privileges. If I understand
> correctly, userstorage solved that by having a setup phase as root
> before running the tests as a normal user? We don't really have that in
> qemu-iotests.
>
> Some tests require passwordless sudo and are skipped otherwise, but this
> means that in practice they are almost always skipped.
>

Yes, this is the assumption the storage is being created before running the
tests,
for example when setting up a development or CI environment, and the tests
can run with unprivileged user.

> But more important, are you ok with the change?
> >
> > I'm not sure about not creating sparse images by default - this is not
> > consistent with qemu-img convert and qemu-nbd, which do sparsify by
> > default. The old behavior seems better.
>
> Well, your patches make it do what we always claimed it would do, so
> that consistency is certainly a good thing. Unmapping on write_zeroes
> and ignoring truncate is a weird combination anyway that doesn't really
> make any sense to me, so I don't think it's worth preserving. The other
> way around could have been more defensible, but that's not how our bug
> works.
>
> Now, if ignoring all discard requests is a good default these days is a
> separate question and I'm not sure really. Maybe discard=unmap should
> be the default (and apply to both discard are write_zeroes, of course).
>

OK, lets limit the scope to fix the code to match the current docs. We can
tweak
the defaults later.

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Mon, Jun 24, 2024 at 7:08 PM Kevin Wolf  wrote:

> Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > Tested using:
> >
> > Hi Nir,
> > This looks like a good candidate for the qemu-iotests test suite. Adding
> > it to the automated tests will protect against future regressions.
> >
> > Please add the script and the expected output to
> > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> >
> > See the existing test cases in tests/qemu-iotests/ and
> > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > others are Python. I think shell makes sense for this test case. You
> > can copy the test framework boilerplate from an existing test case.
>
> 'du' can't be used like this in qemu-iotests because it makes
> assumptions that depend on the filesystem. A test case replicating what
> Nir did manually would likely fail on XFS with its preallocation.
>

This is why I did not try to add a new qemu-iotest yet.


> Maybe we could operate on a file exposed by the FUSE export that is
> backed by qcow2, and then you can use 'qemu-img map' on that qcow2 image
> to verify the allocation status. Somewhat complicated, but I think it
> could work.
>

Do we have examples of using the FUSE export? It sounds complicated but
being able to test on any file system is awesome. The complexity can be
hidden behind simple test helpers.

Another option is to use a specific file system created for the tests, for
example
on a loop device. We used userstorage[1] in ovirt to test on specific file
systems
with known sector size.

But more important, are you ok with the change?

I'm not sure about not creating sparse images by default - this is not
consistent
with qemu-img convert and qemu-nbd, which do sparsify by default. The old
behavior seems better.

[1] https://github.com/nirs/userstorage

Nir

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Mon, Jun 24, 2024 at 7:08 PM Kevin Wolf  wrote:

> Am 24.06.2024 um 17:23 hat Stefan Hajnoczi geschrieben:
> > On Wed, Jun 19, 2024 at 08:43:25PM +0300, Nir Soffer wrote:
> > > Tested using:
> >
> > Hi Nir,
> > This looks like a good candidate for the qemu-iotests test suite. Adding
> > it to the automated tests will protect against future regressions.
> >
> > Please add the script and the expected output to
> > tests/qemu-iotests/test/write-zeroes-unmap and run it using
> > `(cd build && tests/qemu-iotests/check write-zeroes-unmap)`.
> >
> > See the existing test cases in tests/qemu-iotests/ and
> > tests/qemu-iotests/tests/ for examples. Some are shell scripts and
> > others are Python. I think shell makes sense for this test case. You
> > can copy the test framework boilerplate from an existing test case.
>
> 'du' can't be used like this in qemu-iotests because it makes
> assumptions that depend on the filesystem. A test case replicating what
> Nir did manually would likely fail on XFS with its preallocation.
>

This is why I did not try to add a new qemu-iotest yet.


> Maybe we could operate on a file exposed by the FUSE export that is
> backed by qcow2, and then you can use 'qemu-img map' on that qcow2 image
> to verify the allocation status. Somewhat complicated, but I think it
> could work.
>

Do we have examples of using the FUSE export? It sounds complicated but
being able to test on any file system is awesome. The complexity can be
hidden behind simple test helpers.

Another option is to use a specific file system created for the tests, for
example
on a loop device. We used userstorage[1] in ovirt to test on specific file
systems
with known sector size.

But more important, are you ok with the change?

I'm not sure about not creating sparse images by default - this is not
consistent
with qemu-img convert and qemu-nbd, which do sparsify by default. The old
behavior seems better.

[1] https://github.com/nirs/userstorage

Nir

Re: qemu-img convert: Compression can not be disabled when converting from .qcow2 to .raw

[Nir Soffer]

On Fri, Jun 21, 2024 at 5:48 PM Sven Ott  wrote:

> Hi, I want to mount a VM image to a loop device and give it some excess
> space.
>
> To do so, I download a .qcow2 file, add some 0 bytes with truncate, and
> then convert the image from QCOW2 to RAW format with qemu-img convert,
> like so:
>
> ```
>
> GUEST_IMG=focal-server-cloudimg-amd64
>
> wget https://cloud-images.ubuntu.com/focal/current/$GUEST_IMG
>
> truncate -s 5G $GUEST_IMG.img
>

This is not needed, and ineffective...


>
> qemu-img convert -f qcow2 -O raw $GUEST_IMG.img $GUEST_IMG.raw
>

Since the first thing done in this command is truncating the target image
to 0 bytes.

You can use -n to avoid creation of the target image and use your image,
but this is also not
needed.

You can convert the image:

qemu-img convert -f qccow2 -O raw src.qcow2 dst.raw

and then resize the raw image:

qmeu-img resize dst.raw newsize

You can also resize before converting, it does not matter if you resize
before or after.

Note that you will have to grow the pv/lv/filessystem inside the guest to
use the additional space.

Nir

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> - Need to run all block tests
>

Stale note, make check pass

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> - Need to run all block tests
>

Stale note, make check pass

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

Tested using:

$ cat test-unmap.sh
#!/bin/sh

qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/test.raw

echo
echo "defaults - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "discard=off - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=off >/dev/null
du -sh $img

echo
echo "detect-zeros=on - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=on >/dev/null
du -sh $img

echo
echo "detect-zeros=unmap,discard=unmap - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' |  $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=unmap,discard=unmap
>/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

rm $img


Before this change:

$ cat before.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
0 /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
0 /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw
[nsoffer build (consider-discard-option)]$


After this change:

$ cat after.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
1.0M /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
1.0M /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw


Differences:

$ diff -u before.out after.out
--- before.out 2024-06-19 20:24:09.234083713 +0300
+++ after.out 2024-06-19 20:24:20.526165573 +0300
@@ -3,13 +3,13 @@
 1.0M /tmp/test.raw

 defaults - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

 defaults - write actual zeros
 1.0M /tmp/test.raw

 discard=off - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> When opening an image with discard=off, we punch hole in the image when
> writing zeroes, making the image sparse. This breaks users that want to
> ensure that writes cannot fail with ENOSPACE by using fully allocated
> images.
>
> bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
> opened the child without discard=unmap or discard=on. But we don't go
> through this function when accessing the top node. Move the check down
> to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
>
> Issues:
> - We don't punch hole by default, so images are kept allocated. Before
>   this change we punched holes by default. I'm not sure this is a good
>   change in behavior.
> - Need to run all block tests
> - Not sure that we have tests covering unmapping, we may need new tests
> - We may need new tests to cover this change
>
> Signed-off-by: Nir Soffer 
> ---
>
> Changes since v1:
> - Replace the incorrect has_discard change with the right fix
>
> v1 was here:
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
>
>  block/io.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7217cf811b..301514c880 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> int64_t offset, int64_t bytes,
>  /* By definition there is no user buffer so this flag doesn't make
> sense */
> 

Re: [PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

Tested using:

$ cat test-unmap.sh
#!/bin/sh

qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/test.raw

echo
echo "defaults - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "defaults - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img

echo
echo "discard=off - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=off >/dev/null
du -sh $img

echo
echo "detect-zeros=on - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=on >/dev/null
du -sh $img

echo
echo "detect-zeros=unmap,discard=unmap - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' |  $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=unmap,discard=unmap
>/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img

rm $img


Before this change:

$ cat before.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
0 /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
0 /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw
[nsoffer build (consider-discard-option)]$


After this change:

$ cat after.out

defaults - write zeroes
1.0M /tmp/test.raw

defaults - write zeroes unmap
1.0M /tmp/test.raw

defaults - write actual zeros
1.0M /tmp/test.raw

discard=off - write zeroes unmap
1.0M /tmp/test.raw

detect-zeros=on - write actual zeros
1.0M /tmp/test.raw

detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw

discard=unmap - write zeroes
1.0M /tmp/test.raw

discard=unmap - write zeroes unmap
0 /tmp/test.raw


Differences:

$ diff -u before.out after.out
--- before.out 2024-06-19 20:24:09.234083713 +0300
+++ after.out 2024-06-19 20:24:20.526165573 +0300
@@ -3,13 +3,13 @@
 1.0M /tmp/test.raw

 defaults - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

 defaults - write actual zeros
 1.0M /tmp/test.raw

 discard=off - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw

On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer  wrote:

> When opening an image with discard=off, we punch hole in the image when
> writing zeroes, making the image sparse. This breaks users that want to
> ensure that writes cannot fail with ENOSPACE by using fully allocated
> images.
>
> bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
> opened the child without discard=unmap or discard=on. But we don't go
> through this function when accessing the top node. Move the check down
> to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
>
> Issues:
> - We don't punch hole by default, so images are kept allocated. Before
>   this change we punched holes by default. I'm not sure this is a good
>   change in behavior.
> - Need to run all block tests
> - Not sure that we have tests covering unmapping, we may need new tests
> - We may need new tests to cover this change
>
> Signed-off-by: Nir Soffer 
> ---
>
> Changes since v1:
> - Replace the incorrect has_discard change with the right fix
>
> v1 was here:
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
>
>  block/io.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7217cf811b..301514c880 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> int64_t offset, int64_t bytes,
>  /* By definition there is no user buffer so this flag doesn't make
> sense */
> 

[PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images.

bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.

Issues:
- We don't punch hole by default, so images are kept allocated. Before
  this change we punched holes by default. I'm not sure this is a good
  change in behavior.
- Need to run all block tests
- Not sure that we have tests covering unmapping, we may need new tests
- We may need new tests to cover this change

Signed-off-by: Nir Soffer 
---

Changes since v1:
- Replace the incorrect has_discard change with the right fix

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html

 block/io.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t 
offset, int64_t bytes,
 /* By definition there is no user buffer so this flag doesn't make sense */
 if (flags & BDRV_REQ_REGISTERED_BUF) {
 return -EINVAL;
 }
 
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
 /* Invalidate the cached block-status data range if this write overlaps */
 bdrv_bsc_invalidate_range(bs, offset, bytes);
 
 assert(alignment % bs->bl.request_alignment == 0);
 head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
*child, int64_t offset,
 {
 IO_CODE();
 trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 assert_bdrv_graph_readable();
 
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
 return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
-- 
2.45.1

[PATCH v2] Consider discard option when writing zeros

[Nir Soffer]

When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images.

bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.

Issues:
- We don't punch hole by default, so images are kept allocated. Before
  this change we punched holes by default. I'm not sure this is a good
  change in behavior.
- Need to run all block tests
- Not sure that we have tests covering unmapping, we may need new tests
- We may need new tests to cover this change

Signed-off-by: Nir Soffer 
---

Changes since v1:
- Replace the incorrect has_discard change with the right fix

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html

 block/io.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t 
offset, int64_t bytes,
 /* By definition there is no user buffer so this flag doesn't make sense */
 if (flags & BDRV_REQ_REGISTERED_BUF) {
 return -EINVAL;
 }
 
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
 /* Invalidate the cached block-status data range if this write overlaps */
 bdrv_bsc_invalidate_range(bs, offset, bytes);
 
 assert(alignment % bs->bl.request_alignment == 0);
 head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
*child, int64_t offset,
 {
 IO_CODE();
 trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
 assert_bdrv_graph_readable();
 
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
 return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
 /*
-- 
2.45.1

Re: How to stop QEMU punching holes in backing store

[Nir Soffer]

> On 19 Jun 2024, at 8:54, Justin  wrote:
> 
>>I've run strace and I see calls to fallocate with these flags:
>>FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE
>> 
>>I've tried passing these options: discard=off,detect-zeroes=off but
>>this does not help. This is the full set of relevant options I'm
>>using:
>> 
>>-drive file=/vms/vm0/drive,format=raw,if=virtio,discard=off,detect-zeroes=
>>off
>> 
>> You don't need to disable detect-zeros - in my tests it makes dd if=/dev/zero
>> 5 times faster (770 MiB/s -> 3 GiB/s) since zero writes are converted to
>> fallocate(FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE).
>> 
>> The issue seems to be ignoring the discard option when opening the image,
>> and is fixed by this:
>> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
> 
> Thanks. When might this patch (or something similar) be merged?

The patch need more work, when the work is done and qemu maintainers are happy 
it is a good estimate :-)

> 
>> I think the change needs more work to keep the default behavior
>> since most users want sparse images, but it seems to do what you
>> want - keeping images thick.
> 
> It seems that this patch is making the code align more closely with
> the documentation? To, me, it appeared fairly clear that discard=unmap
> would punch holes, and thus the inverse setting would stop hole
> punching.

Punching holes is used both for discard (e.g. fstrim in the guest) and for 
writing zeros.

I think that discard should work only when you set discard=unmap or 
disacard=on, but writing zeros should always punch holes unless you set 
discard=off. I don’t think this behavior is documented now but it should be, at 
least the intent to keep images sparse when possible.

Nir

Re: [PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

> On 19 Jun 2024, at 11:16, Kevin Wolf  wrote:
> 
> Am 18.06.2024 um 23:24 hat Nir Soffer geschrieben:
>> Set has_discard only when BDRV_O_UNMAP is not set. With this users that
>> want to keep their images fully allocated can disable hole punching
>> when writing zeros or discarding using:
>> 
>>   -drive file=thick.img,discard=off
>> 
>> This change is not entirely correct since it changes the default discard
>> behavior.  Previously we always allowed punching holes, but now you have
>> must use discard=unmap|on to enable it. We probably need to add the
>> BDDR_O_UNMAP flag by default.
>> 
>> make check still works, so maybe we don't have tests for sparsifying
>> images, or maybe you need to run special tests that do not run by
>> default. We needs tests for keeping images non-sparse.
>> 
>> Signed-off-by: Nir Soffer 
> 
> So first of all, I agree with you that this patch is wrong. ;-)
> 
> At first, I failed to understand the problem this is trying to solve. I
> put a debug message in handle_aiocb_discard() and tried with which
> options it triggers. [1] To me, this looked exactly like it should be.
> We only try to discard blocks when discard=unmap is given as an option.
> 
> That leaves the case of write_zeroes. And while at the first sight, the
> code looked good, we do seem to have a problem there and it tried to
> unmap even with discard=off.
> 
>> block/file-posix.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index be25e35ff6..acac2abadc 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
>> *options,
>> ret = -EINVAL;
>> goto fail;
>> }
>> #endif /* !defined(CONFIG_LINUX_IO_URING) */
>> 
>> -s->has_discard = true;
>> +s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
>> s->has_write_zeroes = true;
>> 
>> if (fstat(s->fd, ) < 0) {
>> ret = -errno;
>> error_setg_errno(errp, errno, "Could not stat file");
> 
> s->has_discard is about what the host supports, not about the semantics
> of the QEMU block node. So this doesn't feel right to me.
> 
> So for the buggy case, write_zeroes, bdrv_co_pwrite_zeroes() has code
> that considers the case and clears the ~BDRV_REQ_MAY_UNMAP flags:
> 
>if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>flags &= ~BDRV_REQ_MAY_UNMAP;
>}
> 
> But it turns out that we don't necessarily even go through this function
> for the top node which has discard=off, so it can't take effect:
> 
> (gdb) bt
> #0  0x74f2f144 in __pthread_kill_implementation () at /lib64/libc.so 
> <http://libc.so/>.6
> #1  0x74ed765e in raise () at /lib64/libc.so <http://libc.so/>.6
> #2  0x74ebf902 in abort () at /lib64/libc.so <http://libc.so/>.6
> #3  0x5615aff0 in raw_do_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP, blkdev=false) at 
> ../block/file-posix.c:3643
> #4  0x5615557e in raw_co_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/file-posix.c:3655
> #5  0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f4bcf0, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #6  0x560c72f9 in bdrv_aligned_pwritev (child=0x57f51460, 
> req=0x7fffed5ff800, offset=0, bytes=1048576, align=1, qiov=0x0, 
> qiov_offset=0, flags=6) at ../block/io.c:2100
> #7  0x560c6b41 in bdrv_co_do_zero_pwritev (child=0x57f51460, 
> offset=0, bytes=1048576, flags=6, req=0x7fffed5ff800) at ../block/io.c:2183
> #8  0x560c6647 in bdrv_co_pwritev_part (child=0x57f51460, 
> offset=0, bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at 
> ../block/io.c:2283
> #9  0x560c634f in bdrv_co_pwritev (child=0x57f51460, offset=0, 
> bytes=1048576, qiov=0x0, flags=6) at ../block/io.c:2216
> #10 0x560c75b5 in bdrv_co_pwrite_zeroes (child=0x57f51460, 
> offset=0, bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/io.c:2322
> #11 0x56117d24 in raw_co_pwrite_zeroes (bs=0x57f44980, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/raw-format.c:307
> #12 0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f44980, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #13 0x560c72f9 in bdrv_aligned_pwritev (child=0x57f513f0, 
> req=0x7fffed5ffd90, offset=0, bytes=1048576, align=1, qiov=0x0, 
> q

Re: [PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

> On 19 Jun 2024, at 11:16, Kevin Wolf  wrote:
> 
> Am 18.06.2024 um 23:24 hat Nir Soffer geschrieben:
>> Set has_discard only when BDRV_O_UNMAP is not set. With this users that
>> want to keep their images fully allocated can disable hole punching
>> when writing zeros or discarding using:
>> 
>>   -drive file=thick.img,discard=off
>> 
>> This change is not entirely correct since it changes the default discard
>> behavior.  Previously we always allowed punching holes, but now you have
>> must use discard=unmap|on to enable it. We probably need to add the
>> BDDR_O_UNMAP flag by default.
>> 
>> make check still works, so maybe we don't have tests for sparsifying
>> images, or maybe you need to run special tests that do not run by
>> default. We needs tests for keeping images non-sparse.
>> 
>> Signed-off-by: Nir Soffer 
> 
> So first of all, I agree with you that this patch is wrong. ;-)
> 
> At first, I failed to understand the problem this is trying to solve. I
> put a debug message in handle_aiocb_discard() and tried with which
> options it triggers. [1] To me, this looked exactly like it should be.
> We only try to discard blocks when discard=unmap is given as an option.
> 
> That leaves the case of write_zeroes. And while at the first sight, the
> code looked good, we do seem to have a problem there and it tried to
> unmap even with discard=off.
> 
>> block/file-posix.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index be25e35ff6..acac2abadc 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
>> *options,
>> ret = -EINVAL;
>> goto fail;
>> }
>> #endif /* !defined(CONFIG_LINUX_IO_URING) */
>> 
>> -s->has_discard = true;
>> +s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
>> s->has_write_zeroes = true;
>> 
>> if (fstat(s->fd, ) < 0) {
>> ret = -errno;
>> error_setg_errno(errp, errno, "Could not stat file");
> 
> s->has_discard is about what the host supports, not about the semantics
> of the QEMU block node. So this doesn't feel right to me.
> 
> So for the buggy case, write_zeroes, bdrv_co_pwrite_zeroes() has code
> that considers the case and clears the ~BDRV_REQ_MAY_UNMAP flags:
> 
>if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>flags &= ~BDRV_REQ_MAY_UNMAP;
>}
> 
> But it turns out that we don't necessarily even go through this function
> for the top node which has discard=off, so it can't take effect:
> 
> (gdb) bt
> #0  0x74f2f144 in __pthread_kill_implementation () at /lib64/libc.so 
> <http://libc.so/>.6
> #1  0x74ed765e in raise () at /lib64/libc.so <http://libc.so/>.6
> #2  0x74ebf902 in abort () at /lib64/libc.so <http://libc.so/>.6
> #3  0x5615aff0 in raw_do_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP, blkdev=false) at 
> ../block/file-posix.c:3643
> #4  0x5615557e in raw_co_pwrite_zeroes (bs=0x57f4bcf0, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/file-posix.c:3655
> #5  0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f4bcf0, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #6  0x560c72f9 in bdrv_aligned_pwritev (child=0x57f51460, 
> req=0x7fffed5ff800, offset=0, bytes=1048576, align=1, qiov=0x0, 
> qiov_offset=0, flags=6) at ../block/io.c:2100
> #7  0x560c6b41 in bdrv_co_do_zero_pwritev (child=0x57f51460, 
> offset=0, bytes=1048576, flags=6, req=0x7fffed5ff800) at ../block/io.c:2183
> #8  0x560c6647 in bdrv_co_pwritev_part (child=0x57f51460, 
> offset=0, bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at 
> ../block/io.c:2283
> #9  0x560c634f in bdrv_co_pwritev (child=0x57f51460, offset=0, 
> bytes=1048576, qiov=0x0, flags=6) at ../block/io.c:2216
> #10 0x560c75b5 in bdrv_co_pwrite_zeroes (child=0x57f51460, 
> offset=0, bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/io.c:2322
> #11 0x56117d24 in raw_co_pwrite_zeroes (bs=0x57f44980, offset=0, 
> bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/raw-format.c:307
> #12 0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f44980, 
> offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
> #13 0x560c72f9 in bdrv_aligned_pwritev (child=0x57f513f0, 
> req=0x7fffed5ffd90, offset=0, bytes=1048576, align=1, qiov=0x0, 
> q

Re: [External] : Re: Eclipse and Gradle in OpenJFX

[Nir Lisker]

https://github.com/eclipse/buildship/issues/658 is not that relevant
because it describes a problem in Gradle's integration with Eclipse, not
with Buildship, but it's reported on Buildship, so I don't think there's
much to resolve there except for alignment issues. It has since been solved
on Gradle's side. If you go to Preferences > Grade > Experimental features
> Enable module support, the described issue will be resolved (and maybe
even without it because I'm not sure these features are experimental
anymore - Gradle 7 enabled module support by default
https://docs.gradle.org/7.0/release-notes.html#promoted-features).

On Wed, Jun 19, 2024 at 3:28 AM Andy Goryachev 
wrote:

> Thank you Kevin, for this bit of insight.  I see the Buildship issue
> mentioned in one of the comments is still open with no one assigned
>
>
>
> https://github.com/eclipse/buildship/issues/658
>
>
>
> Chances that it will be fixed in a reasonable time frame are slim to
> none.  I would recommend to make the changes (remove the gradle nature from
> eclipse config files) in the meantime, as it allows for clean import of the
> whole thing into Eclipse.
>
>
>
> Once the situation with Buildship changes, we can remove the Eclipse files
> altogether and rely on gradle import per JDK-8223374.
>
>
>
> I am also very interested in Nir's view on the subject.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *Kevin Rushforth 
> *Date: *Tuesday, June 18, 2024 at 16:09
> *To: *Andy Goryachev , Thiago Milczarek Sayão <
> thiago.sa...@gmail.com>
> *Cc: *openjfx-dev@openjdk.org 
> *Subject: *Re: [External] : Re: Eclipse and Gradle in OpenJFX
>
> We also did that for NetBeans, meaning we removed the NetBeans
> IDE-specific files and use their gradle plug-in (with somewhat mixed
> results). See https://bugs.openjdk.org/browse/JDK-8223375
>
> FWIW, there is an open issue to do the same for Eclipse, but I think it
> hasn't been looked at in a while. Nir is the assignee so he will likely
> have some thoughts on this. https://bugs.openjdk.org/browse/JDK-8223374
>
> -- Kevin
>
> On 6/18/2024 1:14 PM, Andy Goryachev wrote:
>
> Interesting, thank you, Thiago.
>
>
>
> Maybe it's just the quality of gradle support in IntelliJ, or complexity
> of the Buildship plug-in in Eclipse.  It never worked for me, and removing
> the gradle nature from Eclipse project files has an added benefit of
> removing an extra dependency.
>
>
>
> -andy
>
>
>
>
>
>
>
> *From: *Thiago Milczarek Sayão 
> 
> *Date: *Tuesday, June 18, 2024 at 12:51
> *To: *Andy Goryachev 
> 
> *Cc: *openjfx-dev@openjdk.org 
> 
> *Subject: *[External] : Re: Eclipse and Gradle in OpenJFX
>
> Andy,
>
>
>
> We kind of did the opposite for Intellij (got rid of the .iml files and
> went for gradle import):
>
>
>
> https://github.com/openjdk/jfx/pull/1009
> <https://urldefense.com/v3/__https:/github.com/openjdk/jfx/pull/1009__;!!ACWV5N9M2RV99hQ!KGyKiKF64SpFYCZE7vMq0nzFAyNGv-gTQJoxy9lGH2dg15mO5dl-ZuQha_yGdjFGH_l740roARzs-f231vB1WSlkRk4$>
>
>
>
> I couldn't get it to detect the manual tests tho. Changing some gradle
> files worked, but would require a deeper review, so we went without it.
>
>
>
> -- Thiago.
>
>
>
> Em ter., 18 de jun. de 2024 às 16:05, Andy Goryachev <
> andy.goryac...@oracle.com> escreveu:
>
> Dear developers:
>
>
>
> Does anyone use gradle in Eclipse (Buildship plug-in) with the OpenJFX
> repo?
>
>
>
> The reason I am asking is that in my experience, the gradle nature in
> OpenJFX is either misconfigured, or obsolete, or both.  There is a rather
> old wiki page [0] which describes the Eclipse setup, though I don't think
> it is correct anymore.  The initial import of the repository in Eclipse
> triggers an internal gradle run which creates/modifies a bunch of
> .classpath and .project files which must be undone before the workspace
> becomes usable.  In any case, only a proper command line gradle build is
> supported anyway.
>
>
>
> I would like to propose removing the gradle nature from Eclipse's .project
> files in OpenJFX.  Once done, the projects can be trivially imported into a
> new workspace with no extra steps required.  This change has no impact on
> command line build whatsoever.
>
>
>
> What do you think?
>
>
>
> Thank you
>
> -andy
>
>
>
>
>
>
>
> *References*
>
>
>
> [1]
> https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-ConfigureEclipsetousethelatestJDK
>
>
>

Re: Eclipse and Gradle in OpenJFX

[Nir Lisker]

It has been a while since I tried a clean import of jfx, but the
instructions in
https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-UsingEclipse
seem correct, unless something changed. What you describe is written there.
Only the initial import requires a gradle build, and after reverting the
project and classpath changes, you can work with ECJ (unless you need tasks
to build native code etc.)

What is worth looking at is the 'eclipse' gradle task that tries to
generate these files according to the gradle files. However, the way the
project is built in gradle is both complicated and very old (compliant with
gradle 2 I think), so I don't know how well that will work. In my modern
projects, the 'eclipse' task works really well and allows me to not check
in the eclipse files: a gradle refresh/synch is run once (does all the
configuration and dependency management), and then running 'eclipse' once
sets up the eclipse side.

On Tue, Jun 18, 2024 at 10:52 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> Andy,
>
> We kind of did the opposite for Intellij (got rid of the .iml files and
> went for gradle import):
>
> https://github.com/openjdk/jfx/pull/1009
>
> I couldn't get it to detect the manual tests tho. Changing some gradle
> files worked, but would require a deeper review, so we went without it.
>
> -- Thiago.
>
> Em ter., 18 de jun. de 2024 às 16:05, Andy Goryachev <
> andy.goryac...@oracle.com> escreveu:
>
>> Dear developers:
>>
>>
>>
>> Does anyone use gradle in Eclipse (Buildship plug-in) with the OpenJFX
>> repo?
>>
>>
>>
>> The reason I am asking is that in my experience, the gradle nature in
>> OpenJFX is either misconfigured, or obsolete, or both.  There is a rather
>> old wiki page [0] which describes the Eclipse setup, though I don't think
>> it is correct anymore.  The initial import of the repository in Eclipse
>> triggers an internal gradle run which creates/modifies a bunch of
>> .classpath and .project files which must be undone before the workspace
>> becomes usable.  In any case, only a proper command line gradle build is
>> supported anyway.
>>
>>
>>
>> I would like to propose removing the gradle nature from Eclipse's
>> .project files in OpenJFX.  Once done, the projects can be trivially
>> imported into a new workspace with no extra steps required.  This change
>> has no impact on command line build whatsoever.
>>
>>
>>
>> What do you think?
>>
>>
>>
>> Thank you
>>
>> -andy
>>
>>
>>
>>
>>
>>
>>
>> *References*
>>
>>
>>
>> [1]
>> https://wiki.openjdk.org/display/OpenJFX/Using+an+IDE#UsinganIDE-ConfigureEclipsetousethelatestJDK
>>
>

Re: How to stop QEMU punching holes in backing store

[Nir Soffer]

On Wed, Jun 5, 2024 at 5:27 PM Justin  wrote:

> Hi. I'm using QEMU emulator version 5.2.0 on Linux. I am using
> thick-provisioned RAW files as the VM backing store. I've found that
> QEMU is punching holes in my RAW files (it's replacing some zero
> blocks with holes), which means that the number of blocks allocated to
> the VM volumes decreases. It keeps doing this; I've manually used
> fallocate(1) to reallocate the full number of blocks to the VM backing
> store files, and sometime later QEMU punches some more holes.
>
> How do I completely disable all hole punching?
>
> The problem with this behavious is that this confuses capacity
> management software into thinking that there is enough free space to
> create more VMs. The file-system for the VM backing stores becomes
> over-committed. Later, when a VM starts writing non-zero data to the
> holes, the VM hangs because QEMU cannot write to the backing store
> because there are no free blocks available. There is no recovery other
> than deleting files, so it basically means one or more VMs have to be
> sacrificed for the greater good.
>

On the other hand, using a thin disk means that storage operations like
copying a disk, backup or writing zeros are much more efficient.

I would check if it is possible to fix the capacity management system to
consider the disk virtual instead of available space.


> I've run strace and I see calls to fallocate with these flags:
> FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE
>
> I've tried passing these options: discard=off,detect-zeroes=off but
> this does not help. This is the full set of relevant options I'm
> using:
>
> -drive
> file=/vms/vm0/drive,format=raw,if=virtio,discard=off,detect-zeroes=off
>

You don't need to disable detect-zeros - in my tests it makes dd
if=/dev/zero
5 times faster (770 MiB/s -> 3 GiB/s) since zero writes are converted to
fallocate(FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE).

The issue seems to be ignoring the discard option when opening the image,
and is fixed by this:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html

I think the change needs more work to keep the default behavior since most
users
want sparse images, but it seems to do what you want - keeping images thick.

Nir

[PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

Set has_discard only when BDRV_O_UNMAP is not set. With this users that
want to keep their images fully allocated can disable hole punching
when writing zeros or discarding using:

   -drive file=thick.img,discard=off

This change is not entirely correct since it changes the default discard
behavior.  Previously we always allowed punching holes, but now you have
must use discard=unmap|on to enable it. We probably need to add the
BDDR_O_UNMAP flag by default.

make check still works, so maybe we don't have tests for sparsifying
images, or maybe you need to run special tests that do not run by
default. We needs tests for keeping images non-sparse.

Signed-off-by: Nir Soffer 
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index be25e35ff6..acac2abadc 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 ret = -EINVAL;
 goto fail;
 }
 #endif /* !defined(CONFIG_LINUX_IO_URING) */
 
-s->has_discard = true;
+s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
 s->has_write_zeroes = true;
 
 if (fstat(s->fd, ) < 0) {
 ret = -errno;
 error_setg_errno(errp, errno, "Could not stat file");
-- 
2.45.1

[PATCH] block/file-posix: Consider discard flag when opening

[Nir Soffer]

Set has_discard only when BDRV_O_UNMAP is not set. With this users that
want to keep their images fully allocated can disable hole punching
when writing zeros or discarding using:

   -drive file=thick.img,discard=off

This change is not entirely correct since it changes the default discard
behavior.  Previously we always allowed punching holes, but now you have
must use discard=unmap|on to enable it. We probably need to add the
BDDR_O_UNMAP flag by default.

make check still works, so maybe we don't have tests for sparsifying
images, or maybe you need to run special tests that do not run by
default. We needs tests for keeping images non-sparse.

Signed-off-by: Nir Soffer 
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index be25e35ff6..acac2abadc 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 ret = -EINVAL;
 goto fail;
 }
 #endif /* !defined(CONFIG_LINUX_IO_URING) */
 
-s->has_discard = true;
+s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
 s->has_write_zeroes = true;
 
 if (fstat(s->fd, ) < 0) {
 ret = -errno;
 error_setg_errno(errp, errno, "Could not stat file");
-- 
2.45.1

[Bug 2063827] Re: Gnome apps segfault in Nvidia Wayland sessions on Noble

[Nir Nachmani Major]

I have Intel CPU and have never installed Steam. I switched to Xorg for
now because between this bug and bug 1876632, Wayland is not usable.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2063827

Title:
  Gnome apps segfault in Nvidia Wayland sessions on Noble

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/egl-wayland/+bug/2063827/+subscriptions


-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Re: RFR: 8311895: CSS Transitions [v23]

[Nir Lisker]

On Wed, 5 Jun 2024 21:59:13 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://www.w3.org/TR/css-transitions-1/).
>> 
>> ### Future enhancements
>> CSS transition support for backgrounds and borders: #1471 
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   link to easing function images in scene

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2111578408

[IPsec] The ESP Echo Protocol document for IPsecME

[Yoav Nir]

Hi, folks

At IETF 119, Jen Likova presented [1] the ESP Echo Protocol draft [2].

The conversation in the room was lively, but did not look like the kind of 
consensus that we just confirm on the list.

So rather than starting an adoption call now, we’d like to encourage people to 
discuss it on the list, to see if we are approaching such a consensus.

Regards,

Yoav on behalf of the chairs

[1] https://youtu.be/n-yH3jtQmdQ?t=1205  (presentation starts at the 20:10 mark)
[2] https://datatracker.ietf.org/doc/draft-colitti-ipsecme-esp-ping/___
IPsec mailing list -- ipsec@ietf.org
To unsubscribe send an email to ipsec-le...@ietf.org

Re: JavaFX 3D Feature request: Ability to set texture wrap mode for PhongMaterial

[Nir Lisker]

Hi Risto,

There is some work on this already here:
https://github.com/openjdk/jfx/pull/1281. I didn't have time to continue
with it, so it was auto-closed. To answer your points specifically:

Hi, sometimes it is useful to clamp texture coordinates that exceed the
> normal 0-1 range, however currently the default behaviour is to repeat them
> and there is no way to configure it.
> It would be useful to have a TextureWrap property in the PhongMaterial
> class that would have options like REPEAT, CLAMP, MIRRORED_REPEAT, but the
> first 2 would be enough.


I think you're talking about two options that are different, yet similar.
There's texture addressing [1] and texture wrapping [2] (showing links for
D3D). Wrapping mode is a render state and is performed before texture
addressing, which is a sampler state.

I am currently trying to replicate the rendering of a custom engine that
> clamps texture coordinates using the JavaFX 3D api however currently it
> doesn't seem like it's possible.
>

You might have a way of doing that with the mesh's texture coordinates.
Also have a look at the FXyz library [3] and this SO question [4].

Alternative would be to have some sort of a Texture class that is part of
> the PhongMaterial which would have that property instead, it could also
> have additional properties like texture filtering mode (as right now there
> is no way to configure that either).


Texture filtering is the first thing that is implemented in the above PR.
Bilinear and nearest-point are available for mag and min filters; mipmap
isn't really working for reasons I couldn't determine. You can try it out;
see my results from August last year [5].
The next step in that PR is figuring out what configuration options go into
this class (more can be added later, and it should probably be an interface
instead of a class anyway). We don't want to cram too many unrelated things
into it, making it a kitchen sink class, so I suggested starting
conservatively. We also need a unified API for the different current and
(possible) future piplines (D3D9, D3D11, OpenGL, Metal, Vulkan), but I did
some of that research already and the piplines agree rather nicely
[6], so I'm more confident on that front. Specifically, finding out if
render states and sampler states both belong there together because this
configuration class is applied *per map* (diffuse, specular, emissive,
normal), not *per material*. By the way, filtering is also a sampler
state, like addressing, so at the very least addressing can be added there.

- Nir

[1]
https://learn.microsoft.com/en-us/windows/win32/direct3d9/texture-addressing-modes
[2]
https://learn.microsoft.com/en-us/windows/win32/direct3d9/texture-wrapping
[3] https://github.com/FXyz/FXyz
[4]
https://stackoverflow.com/questions/49130485/javafx-shape3d-texturing-dont-strectch-the-image
[5] https://mail.openjdk.org/pipermail/openjfx-dev/2023-August/042181.html
[6] https://mail.openjdk.org/pipermail/openjfx-dev/2023-August/042136.html

On Sat, Jun 8, 2024 at 10:52 PM Risto Vaher 
wrote:

> Hi, sometimes it is useful to clamp texture coordinates that exceed the
> normal 0-1 range, however currently the default behaviour is to repeat them
> and there is no way to configure it.
> It would be useful to have a TextureWrap property in the PhongMaterial
> class that would have options like REPEAT, CLAMP, MIRRORED_REPEAT, but the
> first 2 would be enough.
> I am currently trying to replicate the rendering of a custom engine that
> clamps texture coordinates using the JavaFX 3D api however currently it
> doesn't seem like it's possible.
>
> Alternative would be to have some sort of a Texture class that is part of
> the PhongMaterial which would have that property instead, it could also
> have additional properties like texture filtering mode (as right now there
> is no way to configure that either).
>

[Bug 2063827] Re: Gnome Control Center fails to open on Wayland

[Nir Nachmani Major]

I have the same issue with NVIDA GeForce 4080 and driver 535.171.04 on
Ubuntu 24.04 with Wayland. Problem is that if I update to a newer driver
(which fixes the issue) I get another bug with letters and icons
disappearing after return from suspend (described here:
https://askubuntu.com/questions/1512797/most-characters-disappearing-in-
gnome-after-suspend/1516115#1516115)

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2063827

Title:
  Gnome Control Center fails to open on Wayland

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/nvidia-graphics-drivers-535/+bug/2063827/+subscriptions


-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option [v2]

[Nir Lisker]

On Thu, 30 May 2024 22:54:12 GMT, Alexander Matveev  
wrote:

>> This issue is reproducible with and without `--mac-sign`. jpackage will 
>> "_ad-hoc_" sign application bundle when `--mac-sign` is not specified by 
>> using pseudo-identity "_-_". This is why jpackage tries to sign added files 
>> and this is expected behavior by jpackage. "codesign" fails since added 
>> content made application bundle structure invalid. There is nothing we can 
>> do on jpackage side to sign such invalid bundles. As proposed solution we 
>> will output possible reason for "codesign" failure if it fails and 
>> `--app-content` was specified and possible solution. Proposed message: "One 
>> of the possible reason for "codesign" failure is additional content provided 
>> via "--app-content", which made application bundle structure invalid. Make 
>> sure to provide additional content in a way it will not break application 
>> bundle structure, otherwise add additional content as post-processing step."
>> 
>> Example:
>> Lets assume we have "ReadMe" folder with "ReadMe.txt" file in it.
>> 1) jpackage --type app-image -n Test --app-content ReadMe/ReadMe.txt ...
>> "codesign" will fail with "In subcomponent: Test.app/Contents/ReadMe.txt". 
>> This is expected and "ReadMe.txt" placed in "Test.app/Contents" which is 
>> also expected.
>> 2) jpackage --type app-image -n Test --app-content ReadMe ...
>> Works and "ReadMe.txt" will be placed under "Test.app/Contents/ReadMe".
>> 
>> Sample output before fix:
>> 
>> Error: "codesign" failed with following output:
>> Test.app: replacing existing signature
>> Test.app: code object is not signed at all
>> In subcomponent: Test.app/Contents/ReadMe.txt
>> 
>> 
>> Sample output after fix:
>> 
>> "codesign" failed and additional application content was supplied via the 
>> "--app-content" parameter. Probably the additional content broke the 
>> integrity of the application bundle and caused the failure. Ensure content 
>> supplied via the "--app-content" parameter does not break the integrity of 
>> the application bundle, or add it in the post-processing step.
>> Error: "codesign" failed with following output:
>> Test.app: replacing existing signature
>> Test.app: code object is not signed at all
>> In subcomponent: Test.app/Contents/ReadMe.txt
>
> Alexander Matveev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8332110: jpackage tries to sign added files without the --mac-sign option 
> [v2]

I see, but it doesn't say where to put license files, which are usually in the 
root. Do you know where these belong?

-

PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2143012428

Re: RFR: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option

[Nir Lisker]

On Sat, 25 May 2024 01:12:56 GMT, Alexander Matveev 
 wrote:

> > For your example. This almost seems like an Apple bug if you can add a 
> > directory to the Contents directory but not a file?
> 
> Not sure if it is an Apple bug.

Can this be reported to Apple somehow?

-

PR Comment: https://git.openjdk.org/jdk/pull/19377#issuecomment-2142953200

Re: RFR: 8332748: Grammatical errors in animation API docs [v2]

[Nir Lisker]

On Wed, 29 May 2024 15:52:36 GMT, Lukasz Kostyra  wrote:

>> Checked code and fixed the gramatical error in Transition files: 
>> s/interval/intervals
>> 
>> Fill and Stroke Transition had correct grammatical form already, so those 
>> were untouched. I couldn't find any other instances of this error in our 
>> javadoc documentation.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add missing fix for Scale Transition

I think that 1 Reviewer is enough, but give it some time for others to have a 
chance to review.

-

Marked as reviewed by nlisker (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1466#pullrequestreview-2085810041

Re: RFR: 8332748: Grammatical errors in animation API docs

[Nir Lisker]

On Wed, 29 May 2024 06:53:40 GMT, Lukasz Kostyra  wrote:

> Checked code and fixed the gramatical error in Transition files: 
> s/interval/intervals
> 
> Fill and Stroke Transition had correct grammatical form already, so those 
> were untouched. I couldn't find any other instances of this error in our 
> javadoc documentation.

`ScaleTransition` is missing a fix.

-

PR Comment: https://git.openjdk.org/jfx/pull/1466#issuecomment-2137545249

Re: CFV: New OpenJFX Reviewer: John Hendrikx

[Nir Lisker]

Resending the vote on the mailing list because I sent it only to Kevin.

Vote: YES

On Thu, May 23, 2024 at 2:38 AM Nir Lisker  wrote:

> Vote: YES
>
> On Thu, May 23, 2024 at 2:24 AM Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> I hereby nominate John Hendrikx [1] to OpenJFX Reviewer.
>>
>> John is an OpenJFX community member, who has contributed 39 commits [2]
>> to OpenJFX. John regularly participates in code reviews, especially in
>> the areas of JavaFX properties, scene graph and UI controls, offering
>> valuable feedback.
>>
>> Votes are due by June 5, 2024 at 23:59 UTC.
>>
>> Only current OpenJFX Reviewers [3] are eligible to vote on this
>> nomination. Votes must be cast in the open by replying to this mailing
>> list.
>>
>> For Three-Vote Consensus voting instructions, see [4]. Nomination to a
>> project Reviewer is described in [5].
>>
>> Thanks.
>>
>> -- Kevin
>>
>>
>> [1] https://openjdk.org/census#jhendrikx
>>
>> [2]
>>
>> https://github.com/search?q=repo%3Aopenjdk%2Fjfx+author-name%3A%22John+Hendrikx%22=commits
>>
>> [3] https://openjdk.java.net/census#openjfx
>>
>> [4] https://openjdk.java.net/bylaws#three-vote-consensus
>>
>> [5] https://openjdk.java.net/projects#project-reviewer
>>
>>

Re: RFR: 8311895: CSS Transitions [v20]

[Nir Lisker]

On Sun, 26 May 2024 08:08:26 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   documentation change

I reviewed only the API and some of the internal code. Didn't test it. Looks 
good.

I think the CSR needs updating.

-

Marked as reviewed by nlisker (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2079580350
PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2132184809

Re: RFR: 8311895: CSS Transitions [v19]

[Nir Lisker]

On Sat, 25 May 2024 21:39:24 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   added documentation

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 
749:

> 747: The property value is set programmatically
> 748: The property is bound
> 749: The node becomes invisible

I would mention that this relates to the `visible` property and not to the 
`opacity` one (the node is invisible if opacity is 0).

Other than that, looks good.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614945779

Re: RFR: 8311895: CSS Transitions [v18]

[Nir Lisker]

On Sat, 25 May 2024 20:40:39 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address review comments

This is what I would expect, so looks good. Where is this mentioned to the user?

-

PR Comment: https://git.openjdk.org/jfx/pull/870#issuecomment-2131460092

Re: RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Sat, 25 May 2024 21:04:24 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html 
>> line 690:
>> 
>>> 688: changed, it smoothly transitions to the new value over a 
>>> period of time. Implicit transitions are supported
>>> 689: for all primitive types, as well as for types that implement 
>>> javafx.animation.Interpolatable.
>>> 690: Transitions can be defined for any node in the JavaFX scene 
>>> graph with the following properties:
>> 
>> The way this is phrased makes it sound like the node has "the following 
>> properties", not the transition. Maybe move that part:
>> "Transitions with the following properties can be defined for any node in 
>> the JavaFX scene graph", or just add a comma.
>
> I understand that you're saying that `property`, `duration`, 
> `timing-function`, and `delay` are all sub-properties of `transition`.
> 
> However, from a CSS perspective, `transition-property`, 
> `transition-duration`, `transition-timing-function` and `transition-delay` 
> are properties of `Node`, in the same way as `-fx-background-color`, 
> `-fx-background-insets`, etc. are properties of `Node`.
> 
> `transition` is a short-hand property that combines all four properties into 
> one (we don't have a short-hand property for backgrounds yet). I think that 
> both mental models are basically correct (four properties of node, vs. four 
> sub-properties of transition).

I understand. I find it a bit confusing, but OK to leave as is.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614892590

Re: RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Sat, 25 May 2024 20:37:44 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java 
>> line 277:
>> 
>>> 275:  * @since 23
>>> 276:  */
>>> 277: public enum StepPosition {
>> 
>> I think it would be helpful to include (or link to) images that show what 
>> the steps for each option looks like. The verbal description is a bit 
>> technical.
>
> I've included the images that are also used in the CSS reference 
> documentation. Now there are two copies of these images in two different 
> `doc-files` folders, but I guess that's okay.

I think it's fine. Another option is to link to the part of the reference where 
they are.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614872766

Re: RFR: 8311895: CSS Transitions [v2]

[Nir Lisker]

On Mon, 31 Jul 2023 18:10:46 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java 
>> line 319:
>> 
>>> 317:  * The output time value is determined by the {@link StepPosition}.
>>> 318:  *
>>> 319:  * @param intervals the number of intervals in the step 
>>> interpolator
>> 
>> minor: When I see a plural like `intervals` (or `employees`) I think of a 
>> list of objects. Perhaps `intervalCount` would be better?
>
> Doesn't sound better to me, but I'll defer to what most people feel is best.

I somewhat agree with John. I would probably just use `numberOfIntervals`, but 
`intervalCount` of `numIntervals` is also fine.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614560326

Re: RFR: 8311895: CSS Transitions [v17]

[Nir Lisker]

On Fri, 24 May 2024 11:18:35 GMT, Michael Strauß  wrote:

>> Implementation of [CSS 
>> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>> 
>> ### Future enhancements
>> CSS transitions requires all participating objects to implement the 
>> `Interpolatable` interface. For example, targeting `-fx-background-color` 
>> only works if all background-related objects are interpolatable: `Color`, 
>> `BackgroundFill`, and `Background`.
>> 
>> In a follow-up PR, the following types will implement the `Interpolatable` 
>> interface:
>> `LinearGradient`, `RadialGradient`, `Stop`, `Background`, `BackgroundFill`, 
>> `BackgroundImage`, `BackgroundPosition`, `BackgroundSize`, 
>> `BackgroundStroke`, `BorderWidths`, `CornerRadii`, `Insets`.
>> 
>> ### Limitations
>> This implementation supports both shorthand and longhand notations for the 
>> `transition` property. However, due to limitations of JavaFX CSS, mixing 
>> both notations doesn't work:
>> 
>> .button {
>> transition: -fx-background-color 1s;
>> transition-easing-function: linear;
>> }
>> 
>> This issue should be addressed in a follow-up enhancement.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 57 commits:
> 
>  - Merge branch 'refs/heads/master' into feature/css-transitions
>  - extract magic string to named constant
>  - use existing property in test
>  - fixed documentation
>  - Merge branch 'master' into feature/css-transitions
>  - update 'since' tags
>  - Fix javadoc error
>  - Change javadoc comment
>  - Merge branch 'master' into feature/css-transitions
>  - Discard redundant transitions in StyleableProperty impls
>  - ... and 47 more: https://git.openjdk.org/jfx/compare/94aa2b68...a43dee30

I did a quick review of some of the code, mostly the API. I still don't know 
what happens when a value is programmatically set while a css transition is in 
progress. What I understood is that binding the property will not allow the 
transition to start/continue, but didn't see where setting a value was 
mentioned.

Otherwise looks good. I don't intend to review this beyond my current comments, 
so you can integrate once resolved.

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 
688:

> 686: Transitions
> 687: JavaFX supports implicit transitions for properties that 
> are styled by CSS. When a property value is
> 688: changed, it smoothly transitions to the new value over a period 
> of time. Implicit transitions are supported

Maybe not so smoothly when using a step interpolator?

modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 
690:

> 688: changed, it smoothly transitions to the new value over a period 
> of time. Implicit transitions are supported
> 689: for all primitive types, as well as for types that implement 
> javafx.animation.Interpolatable.
> 690: Transitions can be defined for any node in the JavaFX scene graph 
> with the following properties:

The way this is phrased makes it sound like the node has "the following 
properties", not the transition. Maybe move that part:
"Transitions with the following properties can be defined for any node in the 
JavaFX scene graph", or just add a comma.

modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinition.java
 line 40:

> 38:  * @param duration duration of the transition
> 39:  * @param delay delay after which the transition is started; if negative, 
> the transition starts
> 40:  *  immediately, but will appear to have begun at an earlier 
> point in time

Why accept a negative delay? An 
[`Animation`](https://openjfx.io/javadoc/22/javafx.graphics/javafx/animation/Animation.html#getDelay())
 doesn't accept it.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 
277:

> 275:  * @since 23
> 276:  */
> 277: public enum StepPosition {

I think it would be helpful to include (or link to) images that show what the 
steps for each option looks like. The verbal description is a bit technical.

modules/javafx.graphics/src/main/java/javafx/animation/Interpolator.java line 
328:

> 326:  * @since 23
> 327:  */
> 328: public static Interpolator STEPS(int intervals, StepPosition 
> position) {

Static method names shouldn't be named like constants, although `Interpolator` 
does this for other methods already. Not sure if this trend should continue.

-

PR Review: https://git.openjdk.org/jfx/pull/870#pullrequestreview-2078912077
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614833351
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614836286
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614815486
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1614554508
PR Review Comment: 

Re: RFR: 8322964: Optimize performance of CSS selector matching [v9]

[Nir Lisker]

On Mon, 11 Mar 2024 16:54:25 GMT, John Hendrikx  wrote:

>> Improves performance of selector matching in the CSS subsystem. This is done 
>> by using custom set implementation which are highly optimized for the most 
>> common cases where the number of selectors is small (most commonly 1 or 2). 
>> It also should be more memory efficient for medium sized and large 
>> applications which have many style names defined in various CSS files.
>> 
>> Due to the optimization, the concept of `StyleClass`, which was only 
>> introduced to assign a fixed bit index for each unique style class name 
>> encountered, is no longer needed. This is because style classes are no 
>> longer stored in a `BitSet` which required a fixed index per encountered 
>> style class.
>> 
>> The performance improvements are the result of several factors:
>> - Less memory use when only very few style class names are used in selectors 
>> and styles from a large pool of potential styles (a `BitSet` for potentially 
>> 1000 different style names needed 1000 bits (worst case)  as it was not 
>> sparse).
>> - Specialized sets for small number of elements (0, 1, 2, 3-9 and 10+)
>> - Specialized sets are append only (reduces code paths) and can be made read 
>> only without requiring a wrapper
>> - Iterator creation is avoided when doing `containsAll` check thanks to the 
>> inverse function `isSuperSetOf`
>> - Avoids making a copy of the list of style class names to compare (to 
>> convert them to `StyleClass` and put them into a `Set`) -- this copy could 
>> not be cached and was always discarded immediately after...
>> 
>> The overall performance was tested using the JFXCentral application which 
>> displays about 800 nodes on its start page with about 1000 styles in various 
>> style sheets (and which allows to refresh this page easily).  
>> 
>> On JavaFX 20, the fastest refresh speed was 121 ms on my machine.  With the 
>> improvements in this PR, the fastest refresh had become 89 ms.  The speed 
>> improvement is the result of a 30% faster `Scene#doCSSPass`, which takes up 
>> the bulk of the time to refresh the JFXCentral main page (about 100 ms 
>> before vs 70 ms after the change).
>
> John Hendrikx has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move getStyleClassNames to location it was introduced to reduce diff

The code looks good. I didn't test it, but I'm fine with integrating.

-

PR Comment: https://git.openjdk.org/jfx/pull/1316#issuecomment-2129765316

Re: RFR: 8332313: Update code review guidelines [v4]

[Nir Lisker]

On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Clarify need for CSR when API is modified or removed as well as added

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2069892902

Re: RFR: 8332313: Update code review guidelines [v2]

[Nir Lisker]

On Sat, 18 May 2024 14:24:49 GMT, Kevin Rushforth  wrote:

> Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"?

This looks like a good idea.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605798488

Re: RFR: 8332313: Update code review guidelines [v2]

[Nir Lisker]

On Fri, 17 May 2024 14:58:41 GMT, Nir Lisker  wrote:

>> Kevin Rushforth has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 20 additional 
>> commits since the last revision:
>> 
>>  - Wait for re-review if you've pushed a change addressing a substantive 
>> comment
>>  - Typo + remove sentence that invites reformatting PRs
>>  - Wording changes, added example of API addition
>>  - Formatting
>>  - Add item about checking the target branch
>>  - Remove trailing period from previous
>>  - Minor changes regarding syncing from upstream master
>>  - Clarify that GHA tests might fail for a reason unrelated to the PR
>>  - Fix typo
>>
>>"but It" --> "but it"
>>  - Remove bullet about checking the commit message (Skara already checks)
>>  - ... and 10 more: https://git.openjdk.org/jfx/compare/1b088e5b...9cf7f920
>
> README-code-reviews.md line 72:
> 
>> 70: * Focus first on substantive comments rather than stylistic comments
>> 71: * Check whether there is an automated test; if not, ask for one, if it 
>> is feasible
>> 72: * Make sure that the PR has executed the GitHub Actions (GHA) tests; if 
>> they aren't being run, ask the PR author to enable GHA workflows; if the 
>> test fails on some platforms, check whether it is a real bug (sometimes a 
>> job fails becau se of GHA infrastucture changes or we see a spurious GHA 
>> failure)
> 
> becau se -> because

Never mind, got simul-fixed.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605160705

Re: RFR: 8332313: Update code review guidelines [v2]

[Nir Lisker]

On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth  wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX 
>> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>>  guidelines includes guidance for creating, reviewing, and integrating 
>> changes to JavaFX, along with a pointer to a [Code Review 
>> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
>> 
>> This PR updates these guidelines to improve the quality of reviews, with a 
>> goal of improving JavaFX and decreasing the chance of introducing a serious 
>> regression or other critical bug.
>> 
>> The source branch has three commits:
>> 1. Converts the Code Review Policies Wiki page to a 
>> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>>  file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
>> changes to `README-code-reviews.md`)
>> 
>> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
>> the changes starting from the second commit.
>> 
>> The updates are:
>> 
>> * In the Overview section, add a list of items for Reviewers, PR authors, 
>> and sponsoring Committers to verify prior to integration
>> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
>> policies section
>> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
>> review policies section
>> * Update the `CONTRIBUTING.md` page to highlight important requirements
>
> Kevin Rushforth has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 20 additional 
> commits since the last revision:
> 
>  - Wait for re-review if you've pushed a change addressing a substantive 
> comment
>  - Typo + remove sentence that invites reformatting PRs
>  - Wording changes, added example of API addition
>  - Formatting
>  - Add item about checking the target branch
>  - Remove trailing period from previous
>  - Minor changes regarding syncing from upstream master
>  - Clarify that GHA tests might fail for a reason unrelated to the PR
>  - Fix typo
>
>"but It" --> "but it"
>  - Remove bullet about checking the commit message (Skara already checks)
>  - ... and 10 more: https://git.openjdk.org/jfx/compare/1b088e5b...9cf7f920

README-code-reviews.md line 69:

> 67: * Carefully consider the risk of regression
> 68: * Carefully consider any compatibility concerns
> 69: * Check whether it adds any new public or protected API, even implicitly 
> (such as a public method that overrides a protected method, or a class that 
> is moved from a non-exported to an exported package); if it does, indicate 
> that it needs a CSR

I think that if it looks like an oversight (the author forgot about the default 
constructor), it's better to indicate that than to indicate that the PR needs a 
CSR. Maybe something like:
"if it does and it doesn't look like an oversight, indicate that it needs a CSR"

README-code-reviews.md line 72:

> 70: * Focus first on substantive comments rather than stylistic comments
> 71: * Check whether there is an automated test; if not, ask for one, if it is 
> feasible
> 72: * Make sure that the PR has executed the GitHub Actions (GHA) tests; if 
> they aren't being run, ask the PR author to enable GHA workflows; if the test 
> fails on some platforms, check whether it is a real bug (sometimes a job 
> fails becau se of GHA infrastucture changes or we see a spurious GHA failure)

becau se -> because

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605152176
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605147892

Re: RFR: 8332328: [GHA] GitHub Actions build fails on Linux: unable to find gcc-13

[Nir Lisker]

On Thu, 16 May 2024 15:42:05 GMT, Kevin Rushforth  wrote:

> It looks like the list of packages available in the Ubuntu 22.04 GitHub 
> Actions test runner no longer includes `gcc-13` as of yesterday. I didn't 
> find any documentation to indicate that this was intentional, but now that 
> Ubuntu 24.04 is available we can fix the problem by updating to that version. 
> We will need to do this at some point anyway, so we might as well do it now.
> 
> With this fix, the GHA test build now passes on all platforms. See the test 
> results to verify this.
> 
> Given that GHA builds are currently broken on Linux, and that this fix 
> doesn't affect the product at all (just our GHA tests), I will integrate this 
> as soon as it is approved by 1 Reviewer, as an exception to the usual 
> requirement to wait for 24 hours.

Note that the Ubuntu 24 runner is in beta: 
https://github.com/actions/runner-images#available-images.

-

PR Comment: https://git.openjdk.org/jfx/pull/1457#issuecomment-2115686652

Re: RFR: 8332313: Update code review guidelines

[Nir Lisker]

On Wed, 15 May 2024 20:08:10 GMT, John Hendrikx  wrote:

>> or is it?  :-)
>
> Isn't this automatic? Seems weird you could integrate without these passing.

> or is it? :-)

![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293)

Looks to me like it is...

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602201785

Re: RFR: 8332313: Update code review guidelines

[Nir Lisker]

On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth  wrote:

> Update the code review guidelines for JavaFX.
> 
> The JavaFX 
> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md)
>  guidelines includes guidance for creating, reviewing, and integrating 
> changes to JavaFX, along with a pointer to a [Code Review 
> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) Wiki page.
> 
> This PR updates these guidelines to improve the quality of reviews, with a 
> goal of improving JavaFX and decreasing the chance of introducing a serious 
> regression or other critical bug.
> 
> The source branch has three commits:
> 1. Converts the Code Review Policies Wiki page to a 
> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md)
>  file in the repo and updates hyperlinks to the new location.
> 2. Update `README-code-reviews.md` with new guidelines
> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor 
> changes to `README-code-reviews.md`)
> 
> Commit 1 is content neutral, so it might be helpful for reviewers to look at 
> the changes starting from the second commit.
> 
> The updates are:
> 
> * In the Overview section, add a list of items for Reviewers, PR authors, and 
> sponsoring Committers to verify prior to integration
> * Create a "Guidelines for reviewing a PR" subsection of the Code review 
> policies section
> * Create a "Before you integrate or sponsor a PR" subsection of the Code 
> review policies section
> * Update the `CONTRIBUTING.md` page to highlight important requirements

README-code-reviews.md line 48:

> 46: All code reviews must be done via a pull request submitted against this 
> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must 
> exist before the pull request will be reviewed. See 
> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
> request.
> 47: 
> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
> role (aka a "R"eviewer). We have a different code review threshold for 
> different types of changes. If there is disagreement as to whether a fix is 
> low-impact or high-impact, then it is considered high-impact. In other words 
> we will always err on the side of quality by "rounding up" to the next higher 
> category. The contributor can say whether they think something is low-impact 
> or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either 
> adds a comment indicating that they think a single review is sufficient, or 
> else issues the Skara `/reviewers 2` command requesting a second reviewer (a 
> Reviewer can request more than 2 reviewers in some cases where a fix might be 
> especially risky or cut across multiple functional areas).

"but **It** is" -> it

README-code-reviews.md line 48:

> 46: All code reviews must be done via a pull request submitted against this 
> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must 
> exist before the pull request will be reviewed. See 
> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull 
> request.
> 47: 
> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" 
> role (aka a "R"eviewer). We have a different code review threshold for 
> different types of changes. If there is disagreement as to whether a fix is 
> low-impact or high-impact, then it is considered high-impact. In other words 
> we will always err on the side of quality by "rounding up" to the next higher 
> category. The contributor can say whether they think something is low-impact 
> or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either 
> adds a comment indicating that they think a single review is sufficient, or 
> else issues the Skara `/reviewers 2` command requesting a second reviewer (a 
> Reviewer can request more than 2 reviewers in some cases where a fix might be 
> especially risky or cut across multiple functional areas).

About requesting reviews. I think that only some people can request reviews 
through GitHub, I never managed to do it on this repo, probably a matter of 
permissions. Might worth clarifying how this works.

README-code-reviews.md line 58:

> 56: * Determine whether this needs 2 reviewers and whether it needs a CSR; 
> issue the `/reviewers 2` or `/csr` command as needed
> 57: * If you want to indicate your approval, but still feel additional 
> reviewers are needed, you may increase the number of reviewers (e.g., from 2 
> to 3)
> 58: * If you want an area expert to review a PR, indicate this in a comment 
> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can 
> indicate whether or not they plan to review it

Should a list of experts per area be available somewhere? The Wiki has an old 
"code ownership" table that is out of date. Usually you get to know the experts 
only after they have reviewed 

Gradle support

[Nir Lisker]

Hello,

I see that dependency scanning is supported for Maven pom files, however,
many projects use Gradle as their dependency management tool. Has Gradle
support been considered?

Best,
Nir

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex [v3]

[Nir Lisker]

On Fri, 26 Apr 2024 07:53:59 GMT, Lukasz Kostyra  wrote:

>> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
>> no longer needed.
>> 
>> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
>> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
>> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it 
>> was leveraged to transparently use the Ex device in the backend) but now we 
>> don't have the non-Ex device, so that keeps it a bit more consistent and 
>> clear IMO.
>> 
>> Verified by running tests on Windows 11, did not notice any regressions. 
>> Unfortunately I have no way to test this on older systems.
>
> Lukasz Kostyra has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Change pd3dEx to pd3d9

Marked as reviewed by nlisker (Reviewer).

-

PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2024451797

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

Tested the functionality with the 3DLighting app, things look the same as 
before the patch. Left a minor comment.

modules/javafx.graphics/src/main/native-prism-d3d/D3DPipeline.cc line 237:

> 235: }
> 236: 
> 237: int getMaxSampleSupport(IDirect3D9Ex *d3d9, UINT adapter) {

Minor: In some cases you also change the name of the variable to add the "Ex" 
suffix., like in

D3DContext::D3DContext(IDirect3D9Ex *pd3dEx, UINT adapter)
 ^

Here and In `PiplineManager.h` it's left as `IDirect3D9Ex * pd3d9;` without 
"Ex".
I don't mind it either way (I would probably not bother changing them myself), 
but perhaps we should remain consistent.

-

Marked as reviewed by nlisker (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1445#pullrequestreview-2022879147
PR Review Comment: https://git.openjdk.org/jfx/pull/1445#discussion_r1579699815

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

Found the problem. That merge bumped the min JDK to 21, which I was using 
anyway, but it required recompiling the native D3D resources with the new JDK. 
The application works now.

-

PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2077287504

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

I traced the issue to commit id 3914db26f3abb573ed0e320a361477e1d3e7a9ac, which 
is a merge Kevin did ~6 weeks ago. Placing the head on this commit or after 
causes the above error, but moving 1 commit back to "Create release notes for 
JavaFX 22" lets the application start normally.

I understand that this PR has nothing to do with this, I just can't test it 
considering this problem.

-

PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2076940263

Re: RFR: 8320563: Remove D3D9 code paths in favor of D3D9Ex

[Nir Lisker]

On Tue, 23 Apr 2024 10:33:58 GMT, Lukasz Kostyra  wrote:

> JFX minimum requirements guarantee 9Ex availability, so old non-Ex paths are 
> no longer needed.
> 
> In multiple parts (ex. Mesh, Graphics, etc.) where the Device is acquired I 
> changed the type to explicitly use `IDirect3DDevice9Ex`. Technically it 
> doesn't matter much (`IDirect3DDevice9Ex` inherits `IDirect3DDevice` - it was 
> leveraged to transparently use the Ex device in the backend) but now we don't 
> have the non-Ex device, so that keeps it a bit more consistent and clear IMO.
> 
> Verified by running tests on Windows 11, did not notice any regressions. 
> Unfortunately I have no way to test this on older systems.

I tried to run the 3DLighting application and I'm getting an error. However, it 
looks like it's not this patch that is causing it since going back a few 
commits also gives this error. The jfx22 branch doesn't have this issue so it 
will take some investigation to find out what's going on. I'm on Win 10.


#
# A fatal error has been detected by the Java Runtime Environment:
#
#  EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x01b44b9ad260, pid=6332, 
tid=18096
#
# JRE version: OpenJDK Runtime Environment (21.0.1+12) (build 21.0.1+12-29)
# Java VM: OpenJDK 64-Bit Server VM (21.0.1+12-29, mixed mode, sharing, tiered, 
compressed oops, compressed class ptrs, g1 gc, windows-amd64)
# Problematic frame:
# J 170 c2 java.lang.String.length()I java.base@21.0.1 (11 bytes) @ 
0x01b44b9ad260 [0x01b44b9ad220+0x0040]
#
# No core dump will be written. Minidumps are not enabled by default on client 
versions of Windows
#
# An error report file with more information is saved as:
# C:\Users\Nir\git\jfx\tests\performance\3DLighting\hs_err_pid6332.log
[5.830s][warning][os] Loading hsdis library failed
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#

-

PR Comment: https://git.openjdk.org/jfx/pull/1445#issuecomment-2076903265

Re: Wayland

[Nir Lisker]

Not sure it helps with warmup, but marking a foreign function as critical
can improve performance:
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/foreign/Linker.Option.html#critical(boolean)
.

On Mon, Apr 22, 2024 at 10:02 PM Philip Race  wrote:

> No, it wasn't. I didn't even use jextracted code.
> The startup cost is around initialisation of FFM - around 70 ms (IIRC)
> overhead on my MacBook
> Then creation of VarHandles and MethodHandles - 2-5 ms each is what I
> measured, so do these lazily if you can.
> And warmup cost is that it takes about 1 iterations to get code fully
> compiled.
>
> java -XX:+PrintFlagsFinal -version 2>&1 | grep CompileThreshold
>  intx CompileThreshold =
> 1  {pd product} {default}
> double CompileThresholdScaling  =
> 1.00  {product} {default}
> uintx IncreaseFirstTierCompileThresholdAt  =
> 50{product} {default}
>  intx Tier2CompileThreshold=
> 0 {product} {default}
>  intx Tier3CompileThreshold=
> 2000  {product} {default}
>  intx Tier4CompileThreshold=
> 15000 {product} {default}
>
> -phil.
>
>
> On 4/22/24 11:45 AM, Thiago Milczarek Sayão wrote:
>
> I think the startup time might be related to all static symbol lookups.
> So I'm manually including just what is needed:
>
> jextract --output src -t com.sun.glass.wayland.extracted \
>   --header-class-name GlassWayland \
>   `pkg-config --libs glib-2.0 gio-2.0 libportal wayland-client` \
>   `pkg-config --cflags-only-I glib-2.0 gio-2.0 libportal wayland-client` \
>glass-wayland.h \
>--include-function xdp_portal_initable_new \
>--include-function xdp_session_close \
>--include-function xdp_portal_open_file \
>--include-function xdp_portal_open_file_finish \
>--include-function g_object_unref \
>--include-function g_timeout_add \
>--include-function g_add_idle \
>--include-function g_main_loop_run \
>--include-function g_main_loop_new \
>--include-function g_main_loop_ref \
>--include-function g_main_loop_unref \
>--include-function g_main_loop_quit \
>--include-function g_settings_new \
>--include-function g_settings_get_int \
>--include-function wl_display_connect \
>--include-function wl_display_disconnect \
>--include-function wl_display_roundtrip \
>--include-function wl_display_dispatch_pending \
>--include-typedef GAsyncReadyCallback \
>--include-typedef GSourceFunc \
>--include-typedef GError
>
>
> Em seg., 22 de abr. de 2024 às 13:24, Philip Race 
> escreveu:
>
>> As a reminder, using FFM will require all FX *applications* to specify
>> --enable-native-access on the command line
>> Although this is likely coming to JNI soon too.
>>
>> https://docs.oracle.com/en/java/javase/21/core/restricted-methods.html
>>
>> But one thing to watch out for with FFM is startup + warm up time.
>> I struggled a lot with that in using FFM for just one library in the
>> java.desktop module.
>>
>> -phil
>>
>> On 4/22/24 9:12 AM, Nir Lisker wrote:
>>
>> Sorry, we bumped to Java 21 in JavaFX 22 I think since we preserve the
>> N-1 rule.
>>
>> On Mon, Apr 22, 2024 at 6:03 PM Nir Lisker  wrote:
>>
>>> I think that we'll be able to bump to Java 25 in JavaFX 25, like we did
>>> with 21. I suggested initially to bump to Java 22 exactly for FFM as it's
>>> very useful for JavaFX, but was told we shouldn't since it's not an LTS
>>> version.
>>>
>>> I have no idea how long the work on Wayland will take including the code
>>> review (a rather long process), but you should be able to request code
>>> reviews with FFM and have it ready for integration by Java 25.
>>>
>>> On Mon, Apr 22, 2024 at 5:49 PM Thiago Milczarek Sayão <
>>> thiago.sa...@gmail.com> wrote:
>>>
>>>> I was just experimenting, but it seems to be less work than going with
>>>> JNI.
>>>> If I am correct, the next Java LTS will be 25, which will be required
>>>> on JavaFX 29 to be released on September/29.
>>>>
>>>> It's 7 years - that's really too much.
>>>>
>>>> Maybe it's still worthwhile to prototype using FFM and then port
>>>> everything to JNI.
>>>>
>>>> -- Thiago.
>&g

Re: Wayland

[Nir Lisker]

Sorry, we bumped to Java 21 in JavaFX 22 I think since we preserve the N-1
rule.

On Mon, Apr 22, 2024 at 6:03 PM Nir Lisker  wrote:

> I think that we'll be able to bump to Java 25 in JavaFX 25, like we did
> with 21. I suggested initially to bump to Java 22 exactly for FFM as it's
> very useful for JavaFX, but was told we shouldn't since it's not an LTS
> version.
>
> I have no idea how long the work on Wayland will take including the code
> review (a rather long process), but you should be able to request code
> reviews with FFM and have it ready for integration by Java 25.
>
> On Mon, Apr 22, 2024 at 5:49 PM Thiago Milczarek Sayão <
> thiago.sa...@gmail.com> wrote:
>
>> I was just experimenting, but it seems to be less work than going with
>> JNI.
>> If I am correct, the next Java LTS will be 25, which will be required on
>> JavaFX 29 to be released on September/29.
>>
>> It's 7 years - that's really too much.
>>
>> Maybe it's still worthwhile to prototype using FFM and then port
>> everything to JNI.
>>
>> -- Thiago.
>>
>>
>> Em seg., 22 de abr. de 2024 às 11:21, Kevin Rushforth <
>> kevin.rushfo...@oracle.com> escreveu:
>>
>>> Note also that we cannot use Panama in the JavaFX internals yet, since
>>> the minimum version of the JDK is 21.
>>>
>>> -- Kevin
>>>
>>>
>>> On 4/21/2024 10:51 AM, Thiago Milczarek Sayão wrote:
>>> > Hi,
>>> >
>>> > I did a small test app to explore Wayland client and portals (for
>>> > Robot and dialogs such as file open/save).
>>> >
>>> > https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>>> >
>>> > It seems it will work as a glass backend, but some walls will be hit
>>> > on the way :)
>>> >
>>> > I have tried to use jextract (from project Panama) to work directly
>>> > with java, but it seems it does not support wl_ types.
>>> >
>>> > -- Thiago.
>>>
>>>

Re: Wayland

[Nir Lisker]

I think that we'll be able to bump to Java 25 in JavaFX 25, like we did
with 21. I suggested initially to bump to Java 22 exactly for FFM as it's
very useful for JavaFX, but was told we shouldn't since it's not an LTS
version.

I have no idea how long the work on Wayland will take including the code
review (a rather long process), but you should be able to request code
reviews with FFM and have it ready for integration by Java 25.

On Mon, Apr 22, 2024 at 5:49 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> I was just experimenting, but it seems to be less work than going with JNI.
> If I am correct, the next Java LTS will be 25, which will be required on
> JavaFX 29 to be released on September/29.
>
> It's 7 years - that's really too much.
>
> Maybe it's still worthwhile to prototype using FFM and then port
> everything to JNI.
>
> -- Thiago.
>
>
> Em seg., 22 de abr. de 2024 às 11:21, Kevin Rushforth <
> kevin.rushfo...@oracle.com> escreveu:
>
>> Note also that we cannot use Panama in the JavaFX internals yet, since
>> the minimum version of the JDK is 21.
>>
>> -- Kevin
>>
>>
>> On 4/21/2024 10:51 AM, Thiago Milczarek Sayão wrote:
>> > Hi,
>> >
>> > I did a small test app to explore Wayland client and portals (for
>> > Robot and dialogs such as file open/save).
>> >
>> > https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>> >
>> > It seems it will work as a glass backend, but some walls will be hit
>> > on the way :)
>> >
>> > I have tried to use jextract (from project Panama) to work directly
>> > with java, but it seems it does not support wl_ types.
>> >
>> > -- Thiago.
>>
>>

Re: Wayland

[Nir Lisker]

Ah, yes, opaque types are indeed unsupported:
https://github.com/openjdk/jextract/blob/master/doc/GUIDE.md#unsupported-features.
As Jorn said there, if the API exposes methods that use opaque types, then
you wouldn't have a problem. Also, if you have the .c file where they are
defined, jextract can handle them. It could be a bit of a hack though.

I wrote a jextract GUI wrapper with JavaFX, which I tested only on Windows
for now. I will try to get the Linux and Mac versions up soon as well. I
find it very helpful compared to the command line and I think it could help
you with the complex headers there.

Note that jextract generates Java 22 compatible code, which is unusable in
JavaFX for now (we comply with Java 21).

On Mon, Apr 22, 2024 at 1:36 AM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> I mailed the jextract list and Jorn Vernee explained that wayland use
> opaque types, which are just defined as such:
>
> struct wl_registry;
>
> I guess you just pass it around and it's defined in the internal .c file.
> Those are not supported by jextract.
>
> I'll find a way to get around it.
>
> But I've been playing with it all day, it seems very good. I was able to
> generate bindings for:
>
> GMain - for the main loop;
> GSettings - for reading settings;
> XDG Portal - for screen capture, screenshot, file dialogs
>
> It looks like this:
>
> 1) To get a setting
>
> try(var a = Arena.ofConfined()) {
> var mouseSettings = 
> g_settings_new(a.allocateUtf8String("org.gnome.desktop.interface"));
> int size = g_settings_get_int(mouseSettings, 
> a.allocateUtf8String("cursor-size"));
> g_object_unref(mouseSettings);
> return new Size(size, size);
> }
>
>
> 2) Callbacks
>
> @Override
> protected void _invokeLater(Runnable runnable) {
> MemorySegment call = GSourceFunc.allocate(p -> {
> runnable.run();
> return 0;
> }, Arena.ofAuto());
>
> g_idle_add(call, MemorySegment.NULL);
> }
>
>
> It looks correct to me, but untested.
>
> -- Thiago.
>
> Em dom., 21 de abr. de 2024 às 18:15, Nir Lisker 
> escreveu:
>
>> Can you link to where all the headers are? I found some in
>> https://gitlab.freedesktop.org/wayland/wayland/-/tree/main/src, but I
>> couldn't see where wl_registry is defined. From what I see, wl_XYZ types
>> are structs, which are supported.
>>
>> By the way, there's a new guide for jextract at
>> https://github.com/openjdk/jextract/blob/master/doc/GUIDE.md. When
>> working with complex headers (includes upon includes), it's important to
>> specify the correct target header.
>>
>> On Sun, Apr 21, 2024 at 11:35 PM Thiago Milczarek Sayão <
>> thiago.sa...@gmail.com> wrote:
>>
>>> jextract --output src -t org.freedesktop.wayland.client
>>> --header-class-name WlClient `pkg-config --cflags-only-I wayland-client`
>>> `pkg-config --libs wayland-client`  /usr/include/wayland-client.h
>>>
>>> WARNING: Skipping wl_registry (type Declared(wl_registry) is not
>>> supported)
>>>
>>> I would need this to hook the events with wl_registry_add_listener, but
>>> currently the code generation for this is not working.
>>>
>>>
>>>
>>>
>>>
>>> Em dom., 21 de abr. de 2024 às 16:39, Nir Lisker 
>>> escreveu:
>>>
>>>> What are wl_ types? jextract only supports c headers.
>>>>
>>>> On Sun, Apr 21, 2024 at 10:31 PM Thiago Milczarek Sayão <
>>>> thiago.sa...@gmail.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I did a small test app to explore Wayland client and portals (for
>>>>> Robot and dialogs such as file open/save).
>>>>>
>>>>> https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>>>>>
>>>>> It seems it will work as a glass backend, but some walls will be hit
>>>>> on the way :)
>>>>>
>>>>> I have tried to use jextract (from project Panama) to work directly
>>>>> with java, but it seems it does not support wl_ types.
>>>>>
>>>>> -- Thiago.
>>>>>
>>>>

Re: Wayland

[Nir Lisker]

Can you link to where all the headers are? I found some in
https://gitlab.freedesktop.org/wayland/wayland/-/tree/main/src, but I
couldn't see where wl_registry is defined. From what I see, wl_XYZ types
are structs, which are supported.

By the way, there's a new guide for jextract at
https://github.com/openjdk/jextract/blob/master/doc/GUIDE.md. When working
with complex headers (includes upon includes), it's important to specify
the correct target header.

On Sun, Apr 21, 2024 at 11:35 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> jextract --output src -t org.freedesktop.wayland.client
> --header-class-name WlClient `pkg-config --cflags-only-I wayland-client`
> `pkg-config --libs wayland-client`  /usr/include/wayland-client.h
>
> WARNING: Skipping wl_registry (type Declared(wl_registry) is not supported)
>
> I would need this to hook the events with wl_registry_add_listener, but
> currently the code generation for this is not working.
>
>
>
>
>
> Em dom., 21 de abr. de 2024 às 16:39, Nir Lisker 
> escreveu:
>
>> What are wl_ types? jextract only supports c headers.
>>
>> On Sun, Apr 21, 2024 at 10:31 PM Thiago Milczarek Sayão <
>> thiago.sa...@gmail.com> wrote:
>>
>>> Hi,
>>>
>>> I did a small test app to explore Wayland client and portals (for Robot
>>> and dialogs such as file open/save).
>>>
>>> https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>>>
>>> It seems it will work as a glass backend, but some walls will be hit on
>>> the way :)
>>>
>>> I have tried to use jextract (from project Panama) to work directly with
>>> java, but it seems it does not support wl_ types.
>>>
>>> -- Thiago.
>>>
>>

Re: Wayland

[Nir Lisker]

What are wl_ types? jextract only supports c headers.

On Sun, Apr 21, 2024 at 10:31 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> Hi,
>
> I did a small test app to explore Wayland client and portals (for Robot
> and dialogs such as file open/save).
>
> https://github.com/tsayao/wayland-test/blob/main/wayland-test.c
>
> It seems it will work as a glass backend, but some walls will be hit on
> the way :)
>
> I have tried to use jextract (from project Panama) to work directly with
> java, but it seems it does not support wl_ types.
>
> -- Thiago.
>

Re: Possible leak on setOnAction

[Nir Lisker]

I didn't find such memory leaks in my application, though I don't do stage
handling. What I would look at is where the `stage` reference in the lambda
is coming from. You say you have a list of open stages. When you close a
stage, do you remove the references to that stage from all places? What
about the object that is holding the reference `stage` in the lambda?

On Thu, Apr 18, 2024 at 1:58 PM Thiago Milczarek Sayão <
thiago.sa...@gmail.com> wrote:

> Hi,
>
> I'm pretty sure setOnAction is holding references.
>
> I have a "Open Windows" menu on my application where it lists the Stages
> opened and if you click, it calls stage.toFront():
>
> menuItem.seOnAction(e -> stage.toFront())
>
> I had many crash reports, all OOM. I got the hprof files and analyzed them
> - turns out this was holding references to all closed stages.
>
> To fix it, I call setOnAction(null) when the stage is closed.
>
> I will investigate further and provide an example.
>
> -- Thiago.
>

Re: How to configure the default connection for virsh

[Nir Soffer]

On Thu, Apr 18, 2024 at 5:20 PM Daniel P. Berrangé 
wrote:

> On Thu, Apr 18, 2024 at 05:07:24PM +0300, Nir Soffer wrote:
> > We are using minikube vms, which require adding the user to to libvirt
> > group[1].
> > We use `virsh -c qemu:///system` for debugging these vms or related
> libvirt
> > networks.
> >
> > Using virsh without specifying the '-c' argument is a common mistake that
> > leads to
> > trouble, for example creating the libvirt default network incorrectly.
> >
> > I wonder if it is possible to configure virsh so it uses -c
> qemu:///system
> > by default?
>
> The $HOME/.config/libvirt/libvirt.conf client config file can contain
>
>uri_default = "qemu:///system"
>
> Or you can set per shell with
>
>export LIBVIRT_DEFAULT_URI=qemu:///system
>
> Or you can set it just for virsh with
>
>export VIRSH_DEFAULT_CONNECT_URI=qemu:///system
>
> See:
>
>   https://libvirt.org/uri.html#default-uri-choice
>   https://libvirt.org/manpages/virsh.html#environment
>
>
Awesome, thanks!
___
Users mailing list -- users@lists.libvirt.org
To unsubscribe send an email to users-le...@lists.libvirt.org

How to configure the default connection for virsh

[Nir Soffer]

We are using minikube vms, which require adding the user to to libvirt
group[1].
We use `virsh -c qemu:///system` for debugging these vms or related libvirt
networks.

Using virsh without specifying the '-c' argument is a common mistake that
leads to
trouble, for example creating the libvirt default network incorrectly.

I wonder if it is possible to configure virsh so it uses -c qemu:///system
by default?

We know that this is not great, but we don't know a better way to use
minikube
with the kvm2 driver. The minikube vms are started for creating a
development
environment and during CI, and they cannot require a password or any
complicated
setup.

[1] https://github.com/kubernetes/minikube/issues/3467

Nir
___
Users mailing list -- users@lists.libvirt.org
To unsubscribe send an email to users-le...@lists.libvirt.org

Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v3]

[Nir Lisker]

On Thu, 28 Mar 2024 22:21:32 GMT, drmarmac  wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java 
>> line 166:
>> 
>>> 164: sm.startAtomic();
>>> 165: 
>>> 166: final List removed = new 
>>> ArrayList<>(c.getRemovedSize());
>> 
>> I wonder if we should add a check for 0 size here to bypass all this code 
>> and unnecessary object allocations if nothing is removed (same for added)
>
> We certainly could, or maybe use wasRemoved(), but I doubt there will be much 
> impact. I guess it's preferred to keep changes unrelated to the issue 
> minimal, so I'd leave it as it is if everyone's ok with that.

These short circuiting operations tend to be worth it only if it's a critical 
path. The GC will collect the allocations very efficiently these days. I didn't 
check what this code segment is used for, but unless it's looped somewhere, I 
doubt there will by any change.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1543793491

Re: RFR: 8273349: Check uses of Stream::peek in controls and replace as needed [v2]

[Nir Lisker]

On Wed, 27 Mar 2024 23:18:43 GMT, Marius Hanl  wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
>>  line 773:
>> 
>>> 771: .collect(Collectors.toList());
>>> 772: 
>>> 773: sortedNewIndices.forEach(this::set);
>> 
>> Why do the double-iteration pattern here and not do the `peek` operation in 
>> a `forEach` like in the other 2 places?
>
> `forEach` is void, so we can not return a list afterwards.

You don't need to return a list, you create it ahead of time like was done in 
line 167

List indices = new ArrayList<>();

and the add the elements in `forEach`.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1430#discussion_r1542145635

Re: RFR: 8327179: Update the 3D lighting application [v5]

[Nir Lisker]

On Wed, 27 Mar 2024 20:38:58 GMT, Nir Lisker  wrote:

>> Update for the 3D lighting test tool as described in the JBS issue.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added gradle script

Looks like the line specified by jcheck was off.

-

PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2024008625

Re: RFR: 8327179: Update the 3D lighting application [v6]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Whitespace?

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/f063d088..ae471f2e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=05
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=04-05

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: RFR: 8327179: Update the 3D lighting application [v5]

[Nir Lisker]

On Wed, 27 Mar 2024 20:38:58 GMT, Nir Lisker  wrote:

>> Update for the 3D lighting test tool as described in the JBS issue.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added gradle script

I added the application to the gradle build file so it can be hooked up with 
its module dependencies. You can now produce a jar that bundles the resource 
with the classes. You still need to point to the modules at runtime.
Let me know if this is a good enough solution.

The way the project is configured in the main build file is not the best, but 
it will require large changes to do it better.

Not sure what jcheck wants. It's complaining about empty spaces that have a `}` 
appear after them. This is the error: 
https://github.com/openjdk/jfx/pull/1387/files#annotation_19798318258.

-

PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2023946848
PR Comment: https://git.openjdk.org/jfx/pull/1387#issuecomment-2023948839

Re: RFR: 8327179: Update the 3D lighting application [v5]

[Nir Lisker]

> Update for the 3D lighting test tool as described in the JBS issue.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Added gradle script

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1387/files
  - new: https://git.openjdk.org/jfx/pull/1387/files/9b3f0c5e..f063d088

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1387=04
 - incr: https://webrevs.openjdk.org/?repo=jfx=1387=03-04

  Stats: 26 lines in 1 file changed: 26 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jfx/pull/1387.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1387/head:pull/1387

PR: https://git.openjdk.org/jfx/pull/1387

Re: RFR: 8328746: Remove unused imports in demo apps

[Nir Lisker]

On Thu, 21 Mar 2024 21:50:37 GMT, Andy Goryachev  wrote:

> Using Eclipse IDE to remove unused imports in **demo apps** (3D, Ensemble, 
> etc.) and update the copyright year to 2024. Using wildcard for more than 10 
> static imports.
> 
> 
> --
> 
> This is a trivial change (though fairly large), 1 reviewer is probably enough.

I find it helpful to separate the imports by their high-level domain name, 
"java.", "javafx.", "com." etc. It makes it easier to see the "span" or 
requirements of the class.

-

PR Comment: https://git.openjdk.org/jfx/pull/1420#issuecomment-2022856424