This patch adds NTFS Read-Write driver to fs/ntfs3.
Having decades of expertise in commercial file systems development and huge
test coverage, we at Paragon Software GmbH want to make our contribution to
the Open Source Community by providing implementation of NTFS Read-Write
driver for the Linux Kernel.
This is fully functional NTFS Read-Write driver. Current version works with
NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying.
We plan to support this version after the codebase once merged, and add new
features and fix bugs. For example, full journaling support over JBD will be
added in later updates.
The patch is too big to handle it within an e-mail body, so it is available to download
on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch
Signed-off-by: Konstantin Komarov <[email protected]>
On 14.08.20 г. 15:29 ч., Konstantin Komarov wrote:
> This patch adds NTFS Read-Write driver to fs/ntfs3.
>
> Having decades of expertise in commercial file systems development and huge
> test coverage, we at Paragon Software GmbH want to make our contribution to
> the Open Source Community by providing implementation of NTFS Read-Write
> driver for the Linux Kernel.
>
> This is fully functional NTFS Read-Write driver. Current version works with
> NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying.
>
> We plan to support this version after the codebase once merged, and add new
> features and fix bugs. For example, full journaling support over JBD will be
> added in later updates.
>
> The patch is too big to handle it within an e-mail body, so it is available to download
> on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch
>
> Signed-off-by: Konstantin Komarov <[email protected]>
>
So how exactly do you expect someone to review this monstrosity ?
On Fri, Aug 14, 2020 at 12:29:01PM +0000, Konstantin Komarov wrote:
> This patch adds NTFS Read-Write driver to fs/ntfs3.
>
> Having decades of expertise in commercial file systems development and huge
> test coverage, we at Paragon Software GmbH want to make our contribution to
> the Open Source Community by providing implementation of NTFS Read-Write
> driver for the Linux Kernel.
>
> This is fully functional NTFS Read-Write driver. Current version works with
> NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying.
>
> We plan to support this version after the codebase once merged, and add new
> features and fix bugs. For example, full journaling support over JBD will be
> added in later updates.
>
> The patch is too big to handle it within an e-mail body, so it is available to download
> on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch
>
> Signed-off-by: Konstantin Komarov <[email protected]>
In case somebody wants to compile it, this fixup is needed to let 'make
fs/ntfs3/' actually work, besides enabling it in the config.
diff --git a/fs/Makefile b/fs/Makefile
index 1c7b0e3f6daa..b0b4ad8affa0 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_SYSV_FS) += sysv/
obj-$(CONFIG_CIFS) += cifs/
obj-$(CONFIG_HPFS_FS) += hpfs/
obj-$(CONFIG_NTFS_FS) += ntfs/
+obj-$(CONFIG_NTFS3_FS) += ntfs3/
obj-$(CONFIG_UFS_FS) += ufs/
obj-$(CONFIG_EFS_FS) += efs/
obj-$(CONFIG_JFFS2_FS) += jffs2/
diff --git a/fs/ntfs3/Makefile b/fs/ntfs3/Makefile
index 4d4fe198b8b8..d99dd1af43aa 100644
--- a/fs/ntfs3/Makefile
+++ b/fs/ntfs3/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_NTFS3_FS) += ntfs3.o
-ntfs3-objs := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \
+ntfs3-y := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \
index.o attrlist.o record.o attrib.o run.o xattr.o\
upcase.o super.o file.o dir.o namei.o lznt.o\
fslog.o
---
Hi Konstantin,
That's cool :) As Nikolay said it needs a little change to the makefiles
to even build.
Are you also going to publish your own mkfs.ntfs3 tool? I dont think the
existing one would support 64k clusters.
I would recommend to run checkpatch (I see already 87 warnings... some
of it is noise):
$ ./scripts/checkpatch.pl <patch>
And sparse (I dont see much):
$ touch fs/ntfs3/*.[ch] && make C=1
You need a recent build of sparse to do that last one. You can pass your
own sparse bin (make CHECK=~/prog/sparse/sparse C=1)
This will be a good first step.
Have you tried to run the xfstests suite against it?
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
I've tried this using libntfs-3g mkfs.ntfs
# mkfs.ntfs /dev/vb1
# mount -t ntfs3 /dev/vb1 /mnt
This already triggered UBSAN:
================================================================================
UBSAN: object-size-mismatch in fs/ntfs3/super.c:834:16
load of address 000000006ae096b5 with insufficient space
for an object of type 'const char'
CPU: 3 PID: 1248 Comm: mount Not tainted 5.8.0+ #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
Call Trace:
dump_stack+0x78/0xa0
ubsan_epilogue+0x5/0x40
ubsan_type_mismatch_common.cold+0xc8/0xcd
__ubsan_handle_type_mismatch_v1+0x32/0x37
ntfs_fill_super+0x9f/0x13e0
? vsnprintf+0x1ef/0x4f0
mount_bdev+0x193/0x1c0
Which points to:
sb->s_magic = *(unsigned long *)s_magic; /* TODO */
Maybe store ('n'<<32)|('t'<<24)|('f'<<16)|('s'<<8) ?
Seems harmless.
* * *
Then I've tried to copy /etc into it:
# cp -rp /etc /mnt
But this triggered a NULL ptr deref:
BUG: kernel NULL pointer dereference, address: 0000000000000028
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] SMP NOPTI
CPU: 0 PID: 1255 Comm: cp Not tainted 5.8.0+ #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4-rebuilt.opensuse.org 04/01/2014
RIP: 0010:ntfs_insert_security+0x187/0x4a0
Code: 00 48 83 c4 18 85 c0 0f 85 54 01 00 00 48 8b 44 24 50 49 8d b5 d8 01 00 00 8b 54 24 60 83 c3 14 48 89 74 24 30 48 85 c0 74 3a <39> 58 28 0f 84 7e 01 00 00 49 89 e8 48 8d 4c 24 50 4c 89 f2 4c 89
RSP: 0018:ffffac73403dfc58 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000064 RCX: 0000000000000010
RDX: 00000000000000b0 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffff94154ed5fe00 R08: 0000000000000000 R09: 0000000000000001
R10: ffff9415781a6180 R11: 0000000000000003 R12: ffff94155c989800
R13: ffff94151e8d2a38 R14: ffff9415781a6170 R15: ffff9415781173f0
FS: 00007fd19b86e580(0000) GS:ffff94157dc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000028 CR3: 000000001ac2a000 CR4: 0000000000350ef0
Call Trace:
? mark_held_locks+0x49/0x70
? lockdep_hardirqs_on_prepare+0xf7/0x190
? ktime_get_coarse_real_ts64+0x9e/0xd0
? trace_hardirqs_on+0x1c/0xe0
ntfs_create_inode+0x2db/0x11c0
ntfs_mkdir+0x58/0x90
vfs_mkdir+0x109/0x1f0
do_mkdirat+0x81/0x120
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fd19ad54dd7
Code: 00 b8 ff ff ff ff c3 0f 1f 40 00 48 8b 05 b9 70 2c 00 64 c7 00 5f 00 00 00 b8 ff ff ff ff c3 0f 1f 40 00 b8 53 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 91 70 2c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffec3c41588 EFLAGS: 00000206 ORIG_RAX: 0000000000000053
RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fd19ad54dd7
RDX: 00000000000c0001 RSI: 00000000000001c0 RDI: 000055cad585fcf0
RBP: 00007ffec3c41990 R08: 00007ffec3c41b50 R09: 00007fd19ada55c0
R10: 00000000000001ef R11: 0000000000000206 R12: 00007ffec3c41b50
R13: 0000000000000000 R14: 0000000000000000 R15: 00007ffec3c437be
(gdb) list *(ntfs_insert_security+0x187)
0xffffffff814e5097 is in ntfs_insert_security (fs/ntfs3/fsntfs.c:1811).
1806
1807 if (!e)
1808 goto insert_security;
1809
1810 next_security:
1811 if (le32_to_cpu(e->sec_hdr.size) != new_sec_size)
1812 goto skip_read_sds;
1813
1814 err = ntfs_read_run_nb(sbi, &ni->file.run, le64_to_cpu(e->sec_hdr.off),
1815 d_security, new_sec_size, NULL);
(gdb) disas /s ntfs_insert_security
....
1811 if (le32_to_cpu(e->sec_hdr.size) != new_sec_size)
0xffffffff814e5097 <+391>: cmp %ebx,0x28(%rax) <=====
0xffffffff814e509a <+394>: je 0xffffffff814e521e <ntfs_insert_security+782>
(gdb) p/x (int)&((NTFS_DE_SDH*)0)->sec_hdr.size
$4 = 0x28
So I think 'e' is NULL. Not sure how it can happen.
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
On Fri, Aug 14, 2020 at 12:29:01PM +0000, Konstantin Komarov wrote:
> This patch adds NTFS Read-Write driver to fs/ntfs3.
>
> Having decades of expertise in commercial file systems development and huge
> test coverage, we at Paragon Software GmbH want to make our contribution to
> the Open Source Community by providing implementation of NTFS Read-Write
> driver for the Linux Kernel.
>
> This is fully functional NTFS Read-Write driver. Current version works with
> NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying.
>
> We plan to support this version after the codebase once merged, and add new
> features and fix bugs. For example, full journaling support over JBD will be
> added in later updates.
>
> The patch is too big to handle it within an e-mail body, so it is available to download
> on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch
The way you submit the driver does not meet significant number of
requirements documented in https://www.kernel.org/doc/html/latest/process/submitting-patches.html .
so this may lead to ignoring the patch as this puts the burden on the
kernel community to make the merge somehow happen. I don't see kernel
involvement from Paragon, so let me build our half of the bridge.
As I reckon, there is interest to have NTFS implementation that's better
than the existing FUSE based support by NTFS-3G (namely performance), so
let me propose something that might lead to merging the patch
eventually.
1. check existing support in kernel
There is fs/ntfs with read-only support from Tuxera, last change in the
git tree is 3 years ago. The driver lacks read-write support so there's
not 100% functionality duplication but there's still driver for the same
filesystem.
I don't know what's the feature parity, how much the in-kernel driver is
used (or what are business relations of Tuxera and Paragon), compared to
the FUSE-based driver, but ideally there's just one NTFS driver in linux
kernel.
The question I'd ask:
- what are users of current fs/ntfs driver going to lose, if the driver
is going to be completely replaced by your driver
If the answer is 'nothing' then, the straightfowrad plan is to just
replace it. Otherwise, we'll have to figure that out.
2. split the patch
One patch of 27k lines of code is way too much to anybody to look at.
As an example, what worked for the recent EXFAT support, send each new
file as a patch. This will pass the mailinglist size filters and keep
the patches logically separated, so eg. there are smaller patches
implementing interaction with VFS (on the inode or directory level) and
leaving out the other internal stuff of the filesystem.
I'm counting about 20 files, that's acceptable. The last patch, or maybe
a separate patch, adds the actual build and config-time support.
The situation is a bit more complicated as there's an existing driver
and I don't see a clear way how to do the transition from 2
implementations (one intermediate) to just one in the final state. We
have that already with the old VFAT and new EXFAT drivers, and it's
solved on the module level. Just one can be loaded (AFAIK). The same
could be done here but it puts some burden on users to know what driver
to load to get the expected set of features.
3. determine support status
You state intentions to work on the driver and there's a new entry in
MAINTAINERS file with 'Supported', so that's a good sign that it's not
just one-time dump of code. Fixing bugs or adding functionality is
expected.
4. development git tree
Once the filesystem is merged, you'd be getting mails from people
running eg. static checkers, build tests, sending cleanups or other
tree-wide changes. The entry in MAINTAINER file does not point to any
git tree, so that's not clear to me what's the expected way to get the
fixes to Linus' tree or where are people supposed to look for 'is this
fixed already'.
There's maybe more I missed, but hopefully HTH.
d.
On Sat, Aug 15, 2020 at 09:06:42PM +0200, David Sterba wrote:
> There's maybe more I missed, but hopefully HTH.
One thing you missed is adding support to fstests
git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
If it passes that torture test, I think we can have confidence that
this is a really good implementation of a filesystem.
On Friday 14 August 2020 12:29:01 Konstantin Komarov wrote:
> This patch adds NTFS Read-Write driver to fs/ntfs3.
>
> Having decades of expertise in commercial file systems development and huge
> test coverage, we at Paragon Software GmbH want to make our contribution to
> the Open Source Community by providing implementation of NTFS Read-Write
> driver for the Linux Kernel.
>
> This is fully functional NTFS Read-Write driver. Current version works with
> NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying.
>
> We plan to support this version after the codebase once merged, and add new
> features and fix bugs. For example, full journaling support over JBD will be
> added in later updates.
>
> The patch is too big to handle it within an e-mail body, so it is available to download
> on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch
>
> Signed-off-by: Konstantin Komarov <[email protected]>
Hello Konstantin! I agree with David that patch is big and could be
split into smaller patches per file, like exfat driver was reviewed.
When you send a new version of ntfs driver, please add me to CC and I
will try to find some time to do code review. Thanks!
From: Aurélien Aptel <[email protected]>
Sent: Friday, August 14, 2020 5:09 PM
>
> Hi Konstantin,
>
> That's cool :) As Nikolay said it needs a little change to the makefiles
> to even build.
>
> Are you also going to publish your own mkfs.ntfs3 tool? I dont think the
> existing one would support 64k clusters.
Hi Aurélien. Thanks for your feedback. We plan to publish our mkfs.ntfs utility as the open source as well (and possibly fschk.ntfs - after mkfs).
>
> I would recommend to run checkpatch (I see already 87 warnings... some
> of it is noise):
>
> $ ./scripts/checkpatch.pl <patch>
>
> And sparse (I dont see much):
>
> $ touch fs/ntfs3/*.[ch] && make C=1
>
> You need a recent build of sparse to do that last one. You can pass your
> own sparse bin (make CHECK=~/prog/sparse/sparse C=1)
>
> This will be a good first step.
The sparse utility is running against the code, as well as checkpatch.pl. Sprase output is clean now. Checkpatch's somehow important warnings will be fixed in v2 (except maybe typedefs).
>
> Have you tried to run the xfstests suite against it?
>
xfstests are being one of our standard test suites among others. Currently we have the 'generic/339' and 'generic/013' test cases failing, working on it now. Other tests either pass or being skipped (due to missing features e.g. reflink).
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
From: Aurélien Aptel <[email protected]>
Sent: Friday, August 14, 2020 6:30 PM
> I've tried this using libntfs-3g mkfs.ntfs
>
> # mkfs.ntfs /dev/vb1
> # mount -t ntfs3 /dev/vb1 /mnt
>
> This already triggered UBSAN:
> Then I've tried to copy /etc into it:
> ...
> # cp -rp /etc /mnt
>
> But this triggered a NULL ptr deref:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000028
Thanks! This will be fixed in v2. To give some context: we use our mkfs utility in tests, coupled with a Windows-native one as a reference. P.S. Already have extended this approach with the current mkfs.ntfs utility as well.
From: David Sterba <[email protected]>
Sent: Saturday, August 15, 2020 10:07 PM
>
> 1. check existing support in kernel
>
> There is fs/ntfs with read-only support from Tuxera, last change in the
> git tree is 3 years ago. The driver lacks read-write support so there's
> not 100% functionality duplication but there's still driver for the same
> filesystem.
>
> I don't know what's the feature parity, how much the in-kernel driver is
> used (or what are business relations of Tuxera and Paragon), compared to
> the FUSE-based driver, but ideally there's just one NTFS driver in linux
> kernel.
>
> The question I'd ask:
>
> - what are users of current fs/ntfs driver going to lose, if the driver
> is going to be completely replaced by your driver
>
> If the answer is 'nothing' then, the straightfowrad plan is to just
> replace it. Otherwise, we'll have to figure that out.
Hi! Regarding the comparison with fs/ntfs driver - we of course did the analysis. There are two main points which make the difference: full read-write support (including compressed/sparse files) and journal replaying. The one thing which is missing in fs/ntfs3 in the inital patch is the appropriate processing of hybernated volumes (mounting them read-only to avoid corruptions). This, however, is considered as trivial change and will be added either in v2, or in v3. In general, we want to have the community's feedback on the topic, and if it's more suitable for the Linux Kernel not to have two implementations in Kernel, then the 'fs/ntfs' may be replaced.
>
> 2. split the patch
>
> One patch of 27k lines of code is way too much to anybody to look at.
The patch will be splitted in v2 file-wise. Wasn't clear initially which way will be more convenient to review.
> 3. determine support status
>
> You state intentions to work on the driver and there's a new entry in
> MAINTAINERS file with 'Supported', so that's a good sign that it's not
> just one-time dump of code. Fixing bugs or adding functionality is
> expected.
>
> 4. development git tree
>
> Once the filesystem is merged, you'd be getting mails from people
> running eg. static checkers, build tests, sending cleanups or other
> tree-wide changes. The entry in MAINTAINER file does not point to any
> git tree, so that's not clear to me what's the expected way to get the
> fixes to Linus' tree or where are people supposed to look for 'is this
> fixed already'.
The external git repo for the code is currently being prepared, so it's the matter of time to have it. Will add the appropriate links to the MAINTERS once done.
From: David Sterba <[email protected]>
Sent: Friday, August 14, 2020 4:41 PM
> In case somebody wants to compile it, this fixup is needed to let 'make
> fs/ntfs3/' actually work, besides enabling it in the config.
>
> diff --git a/fs/Makefile b/fs/Makefile
> index 1c7b0e3f6daa..b0b4ad8affa0 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_SYSV_FS) += sysv/
> obj-$(CONFIG_CIFS) += cifs/
> obj-$(CONFIG_HPFS_FS) += hpfs/
> obj-$(CONFIG_NTFS_FS) += ntfs/
> +obj-$(CONFIG_NTFS3_FS) += ntfs3/
> obj-$(CONFIG_UFS_FS) += ufs/
> obj-$(CONFIG_EFS_FS) += efs/
> obj-$(CONFIG_JFFS2_FS) += jffs2/
> diff --git a/fs/ntfs3/Makefile b/fs/ntfs3/Makefile
> index 4d4fe198b8b8..d99dd1af43aa 100644
> --- a/fs/ntfs3/Makefile
> +++ b/fs/ntfs3/Makefile
> @@ -5,7 +5,7 @@
>
> obj-$(CONFIG_NTFS3_FS) += ntfs3.o
>
> -ntfs3-objs := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \
> +ntfs3-y := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \
> index.o attrlist.o record.o attrib.o run.o xattr.o\
> upcase.o super.o file.o dir.o namei.o lznt.o\
> fslog.o
Thanks! Indeed these fixups are needed to the patch (lost them during final polishing of the code before submitting). Will be fixed in v2.
> From: David Sterba <[email protected]>
> Sent: Friday, August 14, 2020 4:41 PM
> On Fri, Aug 14, 2020 at 12:29:01PM +0000, Konstantin Komarov wrote:
> > This patch adds NTFS Read-Write driver to fs/ntfs3.
> >
> > Having decades of expertise in commercial file systems development and huge
> > test coverage, we at Paragon Software GmbH want to make our contribution to
> > the Open Source Community by providing implementation of NTFS Read-Write
> > driver for the Linux Kernel.
> >
> > This is fully functional NTFS Read-Write driver. Current version works with
> > NTFS(including v3.1) normal/compressed/sparse files and supports journal replaying.
> >
> > We plan to support this version after the codebase once merged, and add new
> > features and fix bugs. For example, full journaling support over JBD will be
> > added in later updates.
> >
> > The patch is too big to handle it within an e-mail body, so it is available to download
> > on our server: https://dl.paragon-software.com/ntfs3/ntfs3.patch
> >
> > Signed-off-by: Konstantin Komarov <[email protected]>
>
> In case somebody wants to compile it, this fixup is needed to let 'make
> fs/ntfs3/' actually work, besides enabling it in the config.
>
> diff --git a/fs/Makefile b/fs/Makefile
> index 1c7b0e3f6daa..b0b4ad8affa0 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -100,6 +100,7 @@ obj-$(CONFIG_SYSV_FS) += sysv/
> obj-$(CONFIG_CIFS) += cifs/
> obj-$(CONFIG_HPFS_FS) += hpfs/
> obj-$(CONFIG_NTFS_FS) += ntfs/
> +obj-$(CONFIG_NTFS3_FS) += ntfs3/
> obj-$(CONFIG_UFS_FS) += ufs/
> obj-$(CONFIG_EFS_FS) += efs/
> obj-$(CONFIG_JFFS2_FS) += jffs2/
> diff --git a/fs/ntfs3/Makefile b/fs/ntfs3/Makefile
> index 4d4fe198b8b8..d99dd1af43aa 100644
> --- a/fs/ntfs3/Makefile
> +++ b/fs/ntfs3/Makefile
> @@ -5,7 +5,7 @@
>
> obj-$(CONFIG_NTFS3_FS) += ntfs3.o
>
> -ntfs3-objs := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \
> +ntfs3-y := bitfunc.o bitmap.o inode.o fsntfs.o frecord.o \
> index.o attrlist.o record.o attrib.o run.o xattr.o\
> upcase.o super.o file.o dir.o namei.o lznt.o\
> fslog.o
> ---
Hi David, thanks! Indeed these fixups are needed to the patch (lost them during final polishing of the code before submitting). Will be fixed in v2.