Is there any case where the bool is true and the pointer is nil? If not the bool seems a bit redundant. Is there something I'm overlooking?

@phlogistonjohn, no we don't have such a case.
bool will be true if there are more changed blocks entries and is such a case the pointer will never be nil.

IIUC, it seems like we can then simply the return types by dropping the boolean and having the callers test for nil/non-nil in all the cases that were previously using the boolean.

The pointer points to the current set of changed blocks (nil on error), while the boolean clearly indicates if more entries remain. Without the boolean, callers would need to infer that from the pointer and error, which makes the flow less clear.

IIUC, it seems like we can then simply the return types by dropping the boolean and having the callers test for nil/non-nil in all the cases that were previously using the boolean.

The boolean is used by cephfs function to indicate if the user needs to call ReadFileBlockDiff again for more entries.
We should preserve the behavior intended by cephfs team here.

@vshankar had pointed to this test case in ceph repo so that I could understand the intended usage.
Hope it helps clear the confusion.

https://github.com/ceph/ceph/blob/060b69c6605d88ad06bd1a3b81697abff2bd4ca5/src/test/libcephfs/snapdiff.cc#L410-L430

Now, I'm even more confused. There's no boolean in the api we're wrapping it returns ret (an int) and info (input) and blocks (output) arguments.
No bool there at all.

Without the boolean, callers would need to infer that from the pointer and error, which makes the flow less clear

This doesn't seem right to me. Error indicates error, period. At that point the loop should stop and handle the error. If error is not nil the other return value(s) are irrelevant. If there is no error then the client can check the nil-ity of the blocks value or not to determine if the loop needs to continue or not. The extra bool value is redundant.

The ret(an int) indicated by r is used in the following manner:

• r > 0 : there will be more changed blocks returned in the next call
• r == 0: no more changed blocks to be returned
• r < 0: there's an errorr

It is confusing but that's how the return value of the function ceph_file_blockdiff is meant to be used.

The following is testcase for this function from https://github.com/ceph/ceph/blob/060b69c6605d88ad06bd1a3b81697abff2bd4ca5/src/test/libcephfs/snapdiff.cc#L410-L430

    r = 1;
    while (r > 0) {
      ceph_file_blockdiff_changedblocks blocks;
      r = ceph_file_blockdiff(&info, &blocks);
      if (r < 0) {
	std::cerr << " Failed to get next changed block, ret:" << r << std::endl;
	return r;
      }

      int nr_blocks = blocks.num_blocks;
      struct cblock *b = blocks.b;
      while (nr_blocks > 0) {
	std::cout << " == [" << b->offset << "~" << b->len << "] == " << std::endl;
	if (expected) {
	  expected->erase(b->offset, b->len);
	}
	++b;
	--nr_blocks;
      }

      ceph_free_file_blockdiff_buffer(&blocks);
    }

In go-ceph implementation to make it simpler,
a boolean is used to indicate if the next function call needs to be made or not.

	if ret < 0 {
		return false, nil, getError(ret)
	}
	// if ret == 0 indicates there is no more entries after this call.
	noMoreEntries = (ret == 0)

Again,

We should preserve the behavior intended by cephfs team here.

Making any change to the behavior at this go-ceph binding layer may lead to unexpected behavior later on.

Well, if we go for an extra round I hope we’ll discover that num_blocks is 0 and/or b is NULL which I guess is what John was hinting at.

just another thought: Can we think of a More() method to basically return whether more processing is required or not? Let's say we have a boolean noMoreEntries inside FileBlockDiffInfo which is set to true during init and subsequently updated during read based on the return code for ceph_file_blockdiff() inside ReadFileBlockDiff().

Now, I'm even more confused. There's no boolean in the api we're wrapping it returns ret (an int) and info (input) and blocks (output) arguments.
No bool there at all.

Without the boolean, callers would need to infer that from the pointer and error, which makes the flow less clear

This doesn't seem right to me. Error indicates error, period. At that point the loop should stop and handle the error. If error is not nil the other return value(s) are irrelevant. If there is no error then the client can check the nil-ity of the blocks value or not to determine if the loop needs to continue or not. The extra bool value is redundant.

Well, if we go for an extra round I hope we’ll discover that num_blocks is 0 and/or b is NULL which I guess is what John was hinting at.

just another thought: Can we think of a More() method to basically return whether more processing is required or not? Let's say we have a boolean noMoreEntries inside FileBlockDiffInfo which is set to true during init and subsequently updated during read based on the return code for ceph_file_blockdiff() inside ReadFileBlockDiff().