@pmj I asked this on IM but if I stick
reinterpret_cast<int*>(KAUTH_RESULT_DEFER),
in this parameter I get a failure. I'm guessing ref compare instead of value compare.
How do you think we should handle this?

Are you trying to check the input value or simulate setting the output parameter?

Neither is currently supported, and I'm not sure what syntax would really work for this type of thing. The best I can think of is to put the check or output parameter setting straight in the mock function itself.

@pmj that makes sense. I'm not sure we need the to test for that value explicitly since we validated the message was sent. What do you think?
We can certainly test it in the mock if absolutely necessary.

Why did you decide to return 1 here for proc_rele?

Any number will actually due since we don't test the result in code.
The results here will seem a bit odd, but hopefully make a lot more sense when we unit tests CurrentProcessWasSpawnedByRegularUser.

Could you add a comment here describing what PlaceholderValue and _ are for and when to use them?

Sure, that commit was thrown together as quickly as possible so that @jeschu1 had something to work with, I think there are a few things there that could be improved. I'll try and get that done soon.

If you get pressed for time @pmj I can do it

Comment added :-)

Apologies for bringing this up again, but could you add a few more details (and possibly rename PlaceholderValue)? Someone new to the code might think that PlaceholderValue is related to file placeholders based on its name.

Maybe something like:

namespace KextMock
{
    struct ParameterPlaceholderValue
    {};

    // Matches with any parameter value
    // ParameterPlaceholderValue can be used:
    //   - When calling kext functions, for parameters values that do not matter to the test
    //   - When checking if a function was called (i.e. DidCallFunction).  ParameterPlaceholderValue is 
    //     a match for all parameter values.
    extern ParameterPlaceholderValue _;

Updating with suggested comment

Minor naming suggestion:

If the some of the more generic test names were slightly more detailed it might make it easier to find test(s) of interest when looking for specific coverage:

Examples

• testWriteFile -> testWriteDataVnodeOpEmptyFile
• testWriteFileFull -> testWriteDataVnodeOpFullFile

What got me thinking about this was when I read the implementation of testWriteFileFull I was surprised to see that it wasn't checking that the MessageType_KtoU_NotifyFileModified message was sent. After realizing that these first tests were only for vnode operations it made sense, but it got me wondering if the test names should include VnodeOp or FileOp (or perhaps the tests should be in different classes?).

This is a good point. I haven't fully decided on seperate files or the same files for both types. My plan is to revisit decision when I go full force on FileOp tests (seperate PR). Hence for now I'd like to punt the naming to a future (but not distance future PR).

Could we check nextCallSequenceNumber to ensure that the above callback was the only callback called (and that it was only called once)?

This comment doesn't quite match the code (now that we have the more generic XCTAssertFalse(MockCalls::DidCallFunction(ProviderMessaging_TrySendRequestAndWaitForResponse)) check), maybe update it to something like:

// KAUTH_FILEOP_OPEN  should not trigger any callbacks for files that are already flagged as in the virtualization root.

I see this comment was marked as resolved, but it looks like the comment was not updated.

Cause I put the comment on the wrong place. I'm updated with 2 superior comments.

Apologies for bringing this up again, but could you add a few more details (and possibly rename PlaceholderValue)? Someone new to the code might think that PlaceholderValue is related to file placeholders based on its name.

Maybe something like:

namespace KextMock
{
    struct ParameterPlaceholderValue
    {};

    // Matches with any parameter value
    // ParameterPlaceholderValue can be used:
    //   - When calling kext functions, for parameters values that do not matter to the test
    //   - When checking if a function was called (i.e. DidCallFunction).  ParameterPlaceholderValue is 
    //     a match for all parameter values.
    extern ParameterPlaceholderValue _;

I see this comment was marked as resolved, but it looks like the comment was not updated.

If the callSequenceNumber work is planned for a follow up PR, could you file a work item for it?

Unless you're renaming the class to ParameterPlaceholderValue all of the ParameterPlaceholderValue in the comment should be replaced with PlaceholderValue.

Rather than writing the loop yourself you can use std::distance to get the count:

return distance(foundCalls.first, foundCalls.second);

I know some of my code has declarations like this as well, but I'm thinking it's less readable than auto at this point. We don't have to go for auto, there are a few in-between options which I think would also be preferable over the status quo:

• I think the latest C++ standard allows partial type declarations, so in this case, I believe std::pair foundCalls = …; should work. This at least makes it obvious we're getting a pair - and pretty much all operations on STL containers return iterators in some form, so not spelling those out in detail seems sensible.
• Locally Typedef'ing the iterator type to something much shorter.

I think the latest C++ standard allows partial type declarations, so in this case, I believe std::pair foundCalls = …; should work. This at least makes it obvious we're getting a pair - and pretty much all operations on STL containers return iterators in some form, so not spelling those out in detail seems sensible.

Cool I did not know about this feature 👍

I know some of my code has declarations like this as well, but I'm thinking it's less readable than auto at this point.

I agree on this point, and template programming is one spot where things can get messy without auto.

If the above approach does not work out I'm good with using auto here with some descriptive variable names.