Following the conversion here: https://github.com/redis/redis/pull/7783

We want to suggest an API to discover if given flags are supported for a given feature (Context Flags, Key space notification flags, ..)

API suggestion

Introduce an API int RM_IsFlagsSupported(int feature, int flags) that for a set of flags and feature, will return boolean indicating if those flags are supported. The features are:

CONTEXT_FLAGS
KEY_SPACE_NOTIFICATIONS_FLAGS
EVENTS_FLAG

The flags can be either a single flag or a multiple flags combine with OR operation. The function will return true only if all the given flags are supported.

And we can also create a specific macro for each feature to wrap it:

#define IS_CTX_FLAGS_SUPPORTED(flags): RM_IsFlagsSupported(CONTEXT_FLAGS, flags)
#define IS_KEYSPAE_FLAGS_SUPPORTED(flags): RM_IsFlagsSupported(KEY_SPACE_NOTIFICATIONS_FLAGS, flags)
#define IS_EVENT_FLAGS_SUPPORTED(flags): RM_IsFlagsSupported(EVENTS_FLAG, flags)

Regardless, we can add a macro (as suggested by @yossigo) that will tell whether or no a given API function is supported (the macro will simply check if the function pointer is NULL): RMAPI_FUNC_SUPPORTED(functionName)

Let me know what you think.

Comment From: yossigo

@MeirShpilraien We can rely on the fact that enums and flags are only added incrementally, so we can have a redismodule.h last-value indicator which can be returned and tested.

I wish to have a mechanism that is not fragile and does not depend on maintaining any additional information.

E.g.

#define REDISMODULE_CTX_FLAGS_LAST (1<<20)

...

#define IS_CTX_FLAG_SUPPORTED(flag) (RM_GetLastSupportedFlag(CONTEXT_FLAGS)  > flag)

Testing multiple flags is also easy this way, but not possible for enum values (e.g., REDISMODULE_EVENT_...).

Comment From: oranagra

I don't like the first suggestion above since it returns a boolean for a given combination. if you want to test several combinations you need to call it several times. I also rather not have the API rely on the fact flags are sequential or not.

i would suggest:

int RM_GetContextFlagsAll(); // returns an int with all supported flags turned on.

this way the caller can run multiple checks on various combinations of flags, and we don't expose the fact the flags are sequential (the code in redis may still assume that, but not the API/ABI)

i also suggest adding this API (fed from an additional define in version.h):

int RM_GetServerVersion(); // returns an int in the format 0x00MMmmpp; e.g. 0x00060008;

Comment From: yossigo

@oranagra I'm OK with that as well, as long as the full flags mask is auto-computed and changes in redismodule.h don't need to be reflected elsewhere manually.

Comment From: oranagra

i was also thinking of adding a REDISMODULE_CTX_FLAGS_LAST (or _NUM) in redismodule.h. but in my mind there's code in module.c that returns a full mask rather than just return the raw value.

Comment From: yossigo

@oranagra But if we have REDISMODULE_CTX_FLAGS_LAST the module.c code can compute the mask, so no changes beyond redismodule.h are needed.

Comment From: oranagra

i think that putting REDISMODULE_CTX_FLAGS_LAST in module.c apart from the rest of the REDISMODULE_CTX_FLAGS_ flags, would mean that people are likely to forget to update it when adding flags.

i'm hoping modules won't try to actually use it, and i hope the fact that the generated docs will mention the accessor function is enough.

Comment From: MeirShpilraien

Summing up the decision:

  1. Introduce a new API's: RM_GetContextFlagsAll, RM_GetKeyspaceNotificationFlagsAll, RM_GetEventFlagsAll that will return the full flags mask of each feature. The module writer can check base on this value if the Flags he needs are supported or not.
  2. For each flag, introduce a new value on redismodule.h, this value represents the LAST value and should be there as a reminder to update it when a new value is added, also it will be used in the code to calculate the full flags mask (assuming flags are incrementally increasing). In addition, documentation should state that the module writer should not use the LAST flag directly and he should use the GetFlagAll API's.
  3. Introduce a new macro RMAPI_FUNC_SUPPORTED(func) that returns whether or not a function API is supported by comparing it to NULL.
  4. Introduce a new API:int RM_GetServerVersion();, that will return the current Redis version in the format 0x00MMmmpp; e.g. 0x00060008;

@yossigo @oranagra if you approve I can start coding :)

Comment From: oranagra

yeah. i think we agree on these, please go head.

Comment From: oranagra

we also need to come up with some plan about things such as https://github.com/redis/redis/pull/7818 for this specific PR adds a new command, so it can't be introduced in a patch-level release, and the module can use RM_GetServerVersion, but we should at least document that these flags are present only starting a certain version..

but i know @ashtul is cooking another PR that adds flags to RM_HashGet, this kind of change can make it into a patch level release, so counting on RM_GetServerVersion is not very good.