Fix zdb/ztest -o and refactor libzpool::set_global_var #11602

problame · 2021-02-15T12:09:00Z

commit ef7f57852233dd603d022e83505f9791fe2b107b (HEAD -> zil-pmem-tmp, problame/fix-zdb--o)
Author: Christian Schwarz <[email protected]>
Date:   Tue Feb 16 11:14:44 2021 +0100

    ztest: fix -o by calling set_global_var in child processes
    
    Without set_global_var() in the child processes the -o option provides
    little use.
    
    Before this change set_global_var() was called as a side-effect of
    getopt processing which only happens for the parent ztest process.
    
    This change limits the set of options that can be set and makes them
    available to the child through ztest_shared_opts_t.
    
    Future work: support arbitrary option count and length.
    
    Signed-off-by: Christian Schwarz <[email protected]>

commit 0c2eb4c086fd3d0eab7cd6356516c226b3d12ce6
Author: Christian Schwarz <[email protected]>
Date:   Tue Feb 16 12:27:48 2021 +0100

    libzpool: set_global_var: refactor to not modify 'arg'
    
    Also fixes leak of the dlopen handle in the error case.
    
    Signed-off-by: Christian Schwarz <[email protected]>

commit 0b750aaf0564b9be87f3ee4a0ac07cabf951f24f
Author: Christian Schwarz <[email protected]>
Date:   Mon Feb 15 13:02:32 2021 +0100

    libzpool: set_global_var: fix endianness handling (fixes zdb -o )
    
    Without this patch I get the error
    
      Setting global variables is only supported on little-endian systems
    
    when using `zdb -o` on my amd64 machine.
    
    Signed-off-by: Christian Schwarz <[email protected]>

Types of changes

Bug fix (non-breaking change which fixes an issue)
Refactoring

Checklist:

My code follows the OpenZFS code style requirements.
~~I have updated the documentation accordingly.~~
I have read the contributing document.
I have added tests to cover my changes.
~~I have run the ZFS Test Suite with this change applied.~~
All commit messages are properly formatted and contain Signed-off-by.

Notes To Review

I actually don't know what the proper way for compile-time endianness-detection is. But looking around the codebase _ZFS_LITTLE_ENDIAN seems like the way to go. @pzakha ?

problame · 2021-02-15T14:39:05Z

Actually I just noticed that ztest -o is broken as well.

behlendorf · 2021-02-15T18:31:44Z

lib/libzpool/util.c

@@ -159,7 +159,7 @@ set_global_var(char *arg)
 	char *varname = arg, *varval;
 	u_longlong_t val;

-#ifndef _LITTLE_ENDIAN
+#ifndef _ZFS_LITTLE_ENDIAN


It looks like zdb_dump_block in zdb.c needs to be updated as well. The _ZFS prefix was added by commit 5678d3f to resolve a conflict, it looks like we missed a couple instances.

problame · 2021-02-16T10:22:11Z

@behlendorf I pushed two additional fixes for ztest to this branch (unrelated to endianness). They should be committed separately.

I don't feel qualified to fix the endianness #define's in the entire code base.

IMHO there should be a gobal constant / static inline function in zfs_context.h or similar that can be called to determine endianness. Compiler inlining and constant folding will do the rest.

problame · 2021-02-16T10:22:41Z

Requesting review from @pzakha

Without this patch I get the error Setting global variables is only supported on little-endian systems when using `zdb -o` on my amd64 machine. Signed-off-by: Christian Schwarz <[email protected]>

Also fixes leak of the dlopen handle in the error case. Signed-off-by: Christian Schwarz <[email protected]>

behlendorf

This likely ended up broken because we don't have a test case which verifies this works correctly. It'd be nice to add even just a very basic one.

nit: build failure on non-x86_84 platforms

ztest.c: In function ‘process_options’:
ztest.c:929:9: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’ [-Werror=format=]
         "max global var count (%lu) exceeded\n",

cmd/ztest/ztest.c

pzakha

Thanks for fixing this!

cmd/ztest/ztest.c

Without set_global_var() in the child processes the -o option provides little use. Before this change set_global_var() was called as a side-effect of getopt processing which only happens for the parent ztest process. This change limits the set of options that can be set and makes them available to the child through ztest_shared_opts_t. Future work: support arbitrary option count and length. Signed-off-by: Christian Schwarz <[email protected]>

problame · 2021-02-17T09:41:45Z

I addressed all the review comments in the last force-push.

This likely ended up broken because we don't have a test case which verifies this works correctly. It'd be nice to add even just a very basic one.

I think the ztest feature never worked for the child processes. Integration-testing -o for ztest would be tricky, I sadly can't afford the half-day to get that right :/

I don't feel qualified to fix the endianness #define's in the entire code base.

IMHO there should be a gobal constant / static inline function in zfs_context.h or similar that can be called to determine endianness. Compiler inlining and constant folding will do the rest.

What do we do about this?

behlendorf · 2021-02-17T16:18:42Z

IMHO there should be a gobal constant / static inline function in zfs_context.h or similar that can be called to determine endianness.

I'm not sure I follow, this is what the _ZFS_LITTLE_ENDIAN and _ZFS_BIG_ENDIAN defines are for. They're defined in the sys/isa_defs.h file. What am I missing?

problame · 2021-02-17T18:02:29Z

I'm not sure I follow, this is what the _ZFS_LITTLE_ENDIAN and _ZFS_BIG_ENDIAN defines are for. They're defined in the sys/isa_defs.h file. What am I missing?

I think it's a good idea to avoid conditional compilation inside functions wherever possible (at least the Linux kernel style guide agreees https://www.kernel.org/doc/html/v4.10/process/coding-style.html#conditional-compilation ).

=> add a static inline boolean_t is_little_endian() { ... } in sys/isa_defs.h and use that in set_global_var.

Regardless, I think that'd be another PR, most likely not by me. I'll create an issue that references this comment.

problame · 2021-02-18T10:53:13Z

Haha, there is an additional catch to this: ztest should forward the -o flags to zdb. I'll implement this and push it to this branch over the day.

problame · 2021-02-18T11:28:14Z

Pushed:

commit 1454d5994428b35ee05aa8cd6a690a1fa3bf671a (HEAD -> zil-pmem-tmp, problame/fix-zdb--o)
Author: Christian Schwarz <[email protected]>
Date:   Thu Feb 18 12:20:09 2021 +0100

    ztest: propagate -o to the zdb child process
    
    I think this is the behavior that most users expect.
    
    Future work: have a separate flag, e.g., -O, to specify separate
    set_global_vars for the zdb child than for the ztest children.
    
    Signed-off-by: Christian Schwarz <[email protected]>

I think this is the behavior that most users expect. Future work: have a separate flag, e.g., -O, to specify separate set_global_vars for the zdb child than for the ztest children. Signed-off-by: Christian Schwarz <[email protected]>

Also fixes leak of the dlopen handle in the error case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes #11602

Without set_global_var() in the child processes the -o option provides little use. Before this change set_global_var() was called as a side-effect of getopt processing which only happens for the parent ztest process. This change limits the set of options that can be set and makes them available to the child through ztest_shared_opts_t. Future work: support arbitrary option count and length. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes #11602

I think this is the behavior that most users expect. Future work: have a separate flag, e.g., -O, to specify separate set_global_vars for the zdb child than for the ztest children. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes #11602

Without this patch I get the error Setting global variables is only supported on little-endian systems when using `zdb -o` on my amd64 machine. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes #11602

Without this patch I get the error Setting global variables is only supported on little-endian systems when using `zdb -o` on my amd64 machine. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

Also fixes leak of the dlopen handle in the error case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

Without set_global_var() in the child processes the -o option provides little use. Before this change set_global_var() was called as a side-effect of getopt processing which only happens for the parent ztest process. This change limits the set of options that can be set and makes them available to the child through ztest_shared_opts_t. Future work: support arbitrary option count and length. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

I think this is the behavior that most users expect. Future work: have a separate flag, e.g., -O, to specify separate set_global_vars for the zdb child than for the ztest children. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

Without this patch I get the error Setting global variables is only supported on little-endian systems when using `zdb -o` on my amd64 machine. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

Also fixes leak of the dlopen handle in the error case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

Without set_global_var() in the child processes the -o option provides little use. Before this change set_global_var() was called as a side-effect of getopt processing which only happens for the parent ztest process. This change limits the set of options that can be set and makes them available to the child through ztest_shared_opts_t. Future work: support arbitrary option count and length. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

I think this is the behavior that most users expect. Future work: have a separate flag, e.g., -O, to specify separate set_global_vars for the zdb child than for the ztest children. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Pavel Zakharov <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#11602

behlendorf reviewed Feb 15, 2021

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 16, 2021

problame force-pushed the fix-zdb--o branch from 481f4e2 to ef7f578 Compare February 16, 2021 11:32

problame changed the title ~~libzpool: set_global_var: fix endianness handling (fixes zdb -o )~~ libzpool: set_global_var: various fixes Feb 16, 2021

problame changed the title ~~libzpool: set_global_var: various fixes~~ Fix zdb/ztest -o and refactor libzpool::set_global_var Feb 16, 2021

problame added 2 commits February 16, 2021 12:39

libzpool: set_global_var: fix endianness handling (fixes zdb -o )

a928a03

Without this patch I get the error Setting global variables is only supported on little-endian systems when using `zdb -o` on my amd64 machine. Signed-off-by: Christian Schwarz <[email protected]>

libzpool: set_global_var: refactor to not modify 'arg'

838bba1

Also fixes leak of the dlopen handle in the error case. Signed-off-by: Christian Schwarz <[email protected]>

problame force-pushed the fix-zdb--o branch from ef7f578 to 1e976ae Compare February 16, 2021 11:40

behlendorf reviewed Feb 16, 2021

View reviewed changes

cmd/ztest/ztest.c Outdated Show resolved Hide resolved

pzakha approved these changes Feb 16, 2021

View reviewed changes

cmd/ztest/ztest.c Outdated Show resolved Hide resolved

problame force-pushed the fix-zdb--o branch from 417aac2 to ff2aa4a Compare February 17, 2021 09:41

behlendorf approved these changes Feb 17, 2021

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 17, 2021

ahrens approved these changes Feb 17, 2021

View reviewed changes

problame mentioned this pull request Feb 17, 2021

check correct use of endianness macros (e.g. _ZFS_LITTLE_ENDIAN, not _LITTLE_ENDIAN ) #11611

Open

ztest: propagate -o to the zdb child process

fbb58f5

I think this is the behavior that most users expect. Future work: have a separate flag, e.g., -O, to specify separate set_global_vars for the zdb child than for the ztest children. Signed-off-by: Christian Schwarz <[email protected]>

problame force-pushed the fix-zdb--o branch from 1454d59 to fbb58f5 Compare February 18, 2021 11:37

behlendorf closed this in b5fffa1 Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix zdb/ztest -o and refactor libzpool::set_global_var #11602

Fix zdb/ztest -o and refactor libzpool::set_global_var #11602

problame commented Feb 15, 2021 •

edited

Loading

problame commented Feb 15, 2021 •

edited

Loading

behlendorf Feb 15, 2021

problame commented Feb 16, 2021 •

edited

Loading

problame commented Feb 16, 2021

behlendorf left a comment

pzakha left a comment

problame commented Feb 17, 2021

behlendorf commented Feb 17, 2021

problame commented Feb 17, 2021

problame commented Feb 18, 2021

problame commented Feb 18, 2021

Fix zdb/ztest -o and refactor libzpool::set_global_var #11602

Fix zdb/ztest -o and refactor libzpool::set_global_var #11602

Conversation

problame commented Feb 15, 2021 • edited Loading

Types of changes

Checklist:

Notes To Review

problame commented Feb 15, 2021 • edited Loading

behlendorf Feb 15, 2021

Choose a reason for hiding this comment

problame commented Feb 16, 2021 • edited Loading

problame commented Feb 16, 2021

behlendorf left a comment

Choose a reason for hiding this comment

pzakha left a comment

Choose a reason for hiding this comment

problame commented Feb 17, 2021

behlendorf commented Feb 17, 2021

problame commented Feb 17, 2021

problame commented Feb 18, 2021

problame commented Feb 18, 2021

problame commented Feb 15, 2021 •

edited

Loading

problame commented Feb 15, 2021 •

edited

Loading

problame commented Feb 16, 2021 •

edited

Loading