Re: [PATCH v3 04/10] lib/igt_drm_netlink: add get_error_counter support

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 31 Aug 2026, "Koppuravuri, Ravi Kishore" <ravi.kishore.koppuravuri@intel.com> wrote:
> On 31-08-2026 14:18, Jani Nikula wrote:
>> On Mon, 24 Aug 2026, Ravi Kishore Koppuravuri <ravi.kishore.koppuravuri@intel.com> wrote:
>>> +static int init_app_context(struct app_context *ctx)
>>> +{
>>> +	if (!ctx)
>>> +		return -EINVAL;
>>> +
>>> +	ctx->sock = NULL;
>>> +	ctx->node_id = UINT32_MAX;
>>> +	ctx->error_id = UINT32_MAX;
>>> +	ctx->error_value = 0;
>>> +	ctx->last_nl_error = 0;
>>> +	ctx->cmd_done = false;
>>> +	ctx->reply_received = false;
>>> +	ctx->ack_received = false;
>>> +	ctx->response_required = false;
>>> +	ctx->family_id = -1;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void cleanup_app_context(struct app_context *ctx)
>>> +{
>>> +	if (!ctx)
>>> +		return;
>>> +
>>> +	ctx->sock = NULL;
>>> +	ctx->node_id = UINT32_MAX;
>>> +	ctx->error_id = UINT32_MAX;
>>> +	ctx->error_value = 0;
>>> +	ctx->last_nl_error = 0;
>>> +	ctx->cmd_done = false;
>>> +	ctx->reply_received = false;
>>> +	ctx->ack_received = false;
>>> +	ctx->response_required = false;
>>> +	ctx->family_id = -1;
>>> +}

Btw, the two functions above do the exact same thing. Why doesn't one
call the other?

Also, why not memset(ctx, 0, sizeof(*ctx)) and then set the non-zero
ones separately?

>>>   struct app_context {
>>>   	struct nl_sock *sock;
>>> +	uint32_t node_id;
>>> +	uint32_t error_id;
>>> +	uint32_t error_value;
>>> +	int last_nl_error;
>>> +	bool cmd_done;
>>> +	bool reply_received;
>>> +	bool ack_received;
>>> +	bool response_required;
>>>   	int family_id;
>>>   };
>>>   
>>>   void igt_cleanup_nl_socket(struct app_context *ctx);
>>>   int igt_init_nl_socket(struct app_context *ctx);
>>> +int igt_get_error_counter(struct app_context *ctx);
>> This is really inconsistent naming, though.
>
> igt_init_nl_socket() and igt_cleanup_nl_socket() are the library 
> functions to initialize and cleanup the netlink socket.
>
> igt_get_error_counter() is the library function to get the error counter.
>
> "igt_" is the prefix used for all the library functions, as suggested by 
> Kamil.
>
> I am not clear why this naming is inconsistent. Could you please 
> elaborate your suggestion here.

Point is, there are a gazillion functions starting with "igt_", and the
above don't make it any more specific. The "igt get error counter" could
refer to any error counter.

I guess I'd have named all the functions and types to have the
igt_drm_netlink_ prefix. Or similar. And then you immediately know what
this is about, without having to look up the function definitions or
documentation.

BR,
Jani.


-- 
Jani Nikula, Intel