glob: Add blocks support

[bnovkov]

Summary

This change introduces the glob_b function which takes a block instead
of a function pointer.

Relnotes: yes
Sponsored by: Klara, Inc.
Obtained from: https://github.com/apple-oss-distributions/Libc

[kevans]

David historically has views on blocks, lets solicit his opinion as well.

include/glob.h

53–61

Is there any harm in just making this a single member union for !BLOCKS? It might be a little less ugly, at least.

[theraven]

I'm not quite sure what this is doing, but it isn't following the pattern that the rest of libc uses. We have DECLARE_BLOCK to declare a block type in a compiler-agnostic way and then CALL_BLOCK to call it. You don't need most of the #ifdef __BLOCKS__ bits at all, they're all handled by the abstraction layer.

See block_abi.h.

If I understand the API of glob[_b] correctly, the block is not captured after the call, it is simply passed down and invoked. If so, this is *all* that is needed, since you don't need any of the memory-management bits that complicated fts_b.

include/glob.h

53–61

I presume we want to support blocks in the caller even if libc is compiled without blocks support? In that case, this should probably have a void* member.

101

Why is this an error? Is this just a discriminator for the discriminated union?

lib/libc/gen/glob.c

264

What is this doing?

272

This doesn't follow the pattern for other libc things that support blocks. They are compiled unconditionally and use the macros if built with a compiler that doesn't support blocks.

1124

This should use the call-block macro if the compiler for libc does not support blocks.

[theraven]

This is also missing an update to the man page. The HISTORY bit should mention that glob_b first appeared in OS X 10.6.

[bnovkov]

Address @theraven 's comments:

• Replace ifdef blocks with the corresponding block_api.h macros
• Include missing manpage updates

[bnovkov]

In D50485#1152334, @theraven wrote:

I'm not quite sure what this is doing, but it isn't following the pattern that the rest of libc uses. We have DECLARE_BLOCK to declare a block type in a compiler-agnostic way and then CALL_BLOCK to call it. You don't need most of the #ifdef __BLOCKS__ bits at all, they're all handled by the abstraction layer.

See block_abi.h.

Thank you for the feedback, I managed to miss the block_abi.h header. I've updated the diff to use the block_abi.h macros and removed the unnecessary ifdef blocks.

[bnovkov]

include/glob.h

101

Yes, this is just a union discriminator.

lib/libc/gen/glob.c

264

This current glob implementation starts by populating the gl_errfunc and gl_flags members before doing any actual matching, so this snippet was hoisted during the refactor.
This makes it possible for glob_b to set the union discriminator and populate gl_errblk.

[theraven]

LGTM, assuming I understand the memory management correctly. I would be happier if something zeroed the gl_errblk field at the end of the call, because otherwise it is a dangling pointer. I don't *think* there's any code path that can reach it, but it's good practice to ensure that any pointer is either to a valid object or null.

include/glob.h

60

This is fine, it could also use the macros for declaring a block type and then simply use that type.

[des]

lib/libc/gen/glob.3

30

482

lib/libc/gen/glob.c

290–291

1117–1118

1120

You can fold that into the definition.

[bnovkov]

• Address @des 's comments
• Zero gl_{errblk,errfunc} before returning

[des]

include/glob.h

100

gl_flags is an int, so setting _GLOB_ERR_BLOCK makes it negative. You should drop a zero, or change the 8 to a 4 or something.

[bnovkov]

Address @des 's comments - change _GLOB_ERR_BLOCK 's value