Read-only accessor functions

[hreinecke]

Hey,
while checking the accessor functions (and the auto-generator; great job btw !) I've found that accessors will always be symmetric, ie having a 'getter' and a 'setter' function. Which is nice for settings which can be modified at will, but for most settings in eg nvme_ctrl this is not really true; the structure is prepared (during which time the accessor functions work), but then the actual controller is created via nvme_add_ctrl(). And once created (by the kernel) most of the settings are read-only (transport, traddr etc are all immutable).

This raises several asks:

• Can we have a 'readonly' marker in the structure to indicate that only a 'getter' function should be generated?
• Can we separate the options required to create a controller from the options of that controller?

[martin-belanger]

The generator looks for the const keyword to determine whether a setter is required. Of course, the code that populates the struct would need to cast away the const to allow writing to those members.

Would that work?

[igaw]

Adding annotation in the struct makes a lot of sense to me. Currently there are include/exclude lists which are not maintained at the same location as the struct definition is also sub optimal.

What about something like this:

struct nvme_ctrl {
	struct list_node entry;
	struct list_head paths;
	struct list_head namespaces;
	struct nvme_subsystem *s;

	struct nvme_global_ctx *ctx;
	struct nvme_transport_handle *hdl;
	char *name;				// !acc:read
	char *sysfs_dir;
	char *address;			// !acc:read
	char *firmware;			// !acc:read
	char *model;			// !acc:read
	char *state;				// !acc:read
[...]
	char *dhchap_host_key;	// !acc:readwrite
	char *dhchap_ctrl_key;	// !acc:readwrite

Second question, I don't think I really understand what you mean here.

[shroffni]

But we still need for those getter functions (irrespective whether it's readonly or both readable and writable) to fetch/retrieve the current value instead of cached one (assuming we don't need separate API for retrieving the non-cached value). So is it possible to auto-generate getter function such that it always return current value?

[hreinecke]

The generator looks for the const keyword to determine whether a setter is required. Of course, the code that populates the struct would need to cast away the const to allow writing to those members.

Would that work?

I like the idea with the 'const' keyword. Let me play around with that.

[hreinecke]

And indeed, that worked. Check #3193.

[hreinecke]

Now we only need to figure out what to do with the 'bool' attributes; not sure if 'const' works there, too.

[hreinecke]

But we still need for those getter functions (irrespective whether it's readonly or both readable and writable) to fetch/retrieve the current value instead of cached one (assuming we don't need separate API for retrieving the non-cached value). So is it possible to auto-generate getter function such that it always return current value?

But that's exactly the point. 'readonly' attributes cannot change, so having a 'setter' function is pointless.

[shroffni]

But we still need for those getter functions (irrespective whether it's readonly or both readable and writable) to fetch/retrieve the current value instead of cached one (assuming we don't need separate API for retrieving the non-cached value). So is it possible to auto-generate getter function such that it always return current value?

But that's exactly the point. 'readonly' attributes cannot change, so having a 'setter' function is pointless.

Hmm what about some of attributes, for instance, controller "state". This is readonly attribute but kernel could still change the "state" value. Isn't it?

[hreinecke]

But we still need for those getter functions (irrespective whether it's readonly or both readable and writable) to fetch/retrieve the current value instead of cached one (assuming we don't need separate API for retrieving the non-cached value). So is it possible to auto-generate getter function such that it always return current value?

But that's exactly the point. 'readonly' attributes cannot change, so having a 'setter' function is pointless.

Hmm what about some of attributes, for instance, controller "state". This is readonly attribute but kernel could still change the "state" value. Isn't it?

That is not what I meant with 'readonly'. In this context 'readonly' means that the variable cannot change once the controller is instantiated, eg the 'cntlid' setting will never change during the lifetime of the controller. Other attributes (like 'state', or 'iopolicy') certainly can change, but that is independent on the 'readonly' setting of the sysfs attribute.

[shroffni]

But we still need for those getter functions (irrespective whether it's readonly or both readable and writable) to fetch/retrieve the current value instead of cached one (assuming we don't need separate API for retrieving the non-cached value). So is it possible to auto-generate getter function such that it always return current value?

But that's exactly the point. 'readonly' attributes cannot change, so having a 'setter' function is pointless.

Hmm what about some of attributes, for instance, controller "state". This is readonly attribute but kernel could still change the "state" value. Isn't it?

That is not what I meant with 'readonly'. In this context 'readonly' means that the variable cannot change once the controller is instantiated, eg the 'cntlid' setting will never change during the lifetime of the controller. Other attributes (like 'state', or 'iopolicy') certainly can change, but that is independent on the 'readonly' setting of the sysfs attribute.

Yes, agreed. My question was specifically about attributes whose values can change during the lifetime of the object. For such attributes, should we consider handling them differently at the API level?

I can think of two possible approaches:

1. Exclude such attributes (e.g., controller state, subsystem iopolicy, and others that may change dynamically) from generate-accessors-exclude.list, so that the accessor does not auto-generate getter/setter functions for them. We can then provide explicit implementations for these attributes in libnvme/src/nvme/tree.c.

2. If we want to retain the cached API variants as well, we could introduce separate non-cached APIs.
   For example, keep the existing auto-generated accessor, nvme_subsystem_get_iopolicy()
   and add a new API such as: nvme_subsystem_get_current_iopolicy(), which would always return the latest (non-cached) value.

My preference would be approach (1), but I’d be interested to hear your thoughts.

[igaw]

@shroffni do you have a 'complete' list of getters which need to retrieve current values? Are all of them directly read from sysfs? If so we might be able generate the code as well. Though we might end up in overengineering land.

Thus your 1) suggestion is gives the quickest result and avoids complex generator logic. Also it could mean we can't generate anything... that would be a bit sad.

Again, adding explicit annotation is my preferred solution because it avoids logic in the generator and type casts in the code base.

[shroffni]

@igaw yes, if we could use generator logic to auto-generate non-cached version of the code then it'd be good. And yes all of those attributes is a direct read from sysfs. Currently the list of attributes for which I'm looking for non-cached version of the API is shown below:

• nvme_ns::ana_state
• nvme_subsystem::iopolicy
• nvme_ns::numa_nodes

Then there're few more attributes which are currently not supported by libnvme, but those are under review for kernel changes upstream here: https://lore.kernel.org/all/20260220175024.292898-1-nilay@linux.ibm.com

Once the above change is merged, I'd also add following counters under libnvme:

• nvme_ns::command_retries
• nvme_ns::command_error_count
• nvme_path::multipath_failover_count
• nvme_ctrl::command_error_count
• nvme_ns_head::requeue_no_usable_path
• nvme_ns_head::fail_no_available_path
• nvme_ctrl::reset_events
• nvme_ctrl::reconnect_events

IMO, if we're able to auto-generate the accessor function for non-cached attribute value then we could easily integrate above counters as well.

And then I do see few APIs added in generate-accessors-exclude.list (maybe due to the current version of the accessor can't auto-generate non-cached API):

• nvme_ctrl::state
• nvme_path::queue_depth

If we can auto-create non-cached API then I think we should remove above two exceptions.

[martin-belanger]

And then I do see few APIs added in generate-accessors-exclude.list (maybe due to the current version of the accessor can't auto-generate non-cached API):

• nvme_ctrl::state
• nvme_path::queue_depth

If we can auto-create non-cached API then I think we should remove above two exceptions.

Yes—these are good examples of attributes that cannot be auto-generated with the current approach. For instance:

const char *nvme_ctrl_get_state(nvme_ctrl_t c)
{
	char *state = c->state;

	c->state = nvme_get_ctrl_attr(c, "state");
	free(state);
	return c->state;
}

In this case, the generator has no way of inferring that it needs to call nvme_get_ctrl_attr(c, "state") to fetch the value from sysfs.

One could argue that this could be addressed via annotations (e.g., //!readonly,sysfs="state"), but that quickly becomes brittle and significantly increases the complexity of the parser.

[shroffni]

@martin-belanger thanks!

@igaw how would you like to proceed? Does option #1 look good in this case?

[igaw]

I'd like to avoid overengineer this problem. @hreinecke idea to generate only the getters (excluding the non trivial) and write the handful setters seems to be the '80%' case.

And for the non-cached version I also suggest to write them per hand for now. We can revisit this later if necessary.

The main goal of the generator was to get rid of the boring, repetive boiler plate code. Anything else can be written per hand. WDYT?