Read only accessors

[hreinecke]

Quite some of the 'host', 'subsystem', and 'ctrl' attributes are actually read-only, and cannot be changed during the lifetime of the controller. So mark these attributes as 'const' and re-generate the accessor functions to drop any 'setter' functions for these fields.

[martin-belanger]

Looks good to me

[hreinecke]

But I guess you have to fixup the accessor-generator; while running it when applying the first patch everything's cool, but running it after the second patch it complains about missing setter functions which had been removed by the first patch. @martin-belanger , can you check what is going on?

[igaw]

I don't like this approach, because having casting of types is often hiding problems. The compilers and likely static analyzer can't work correctly anymore. The implicit annotation approach has it's limit.

[hreinecke]

I don't like this approach, because having casting of types is often hiding problems. The compilers and likely static analyzer can't work correctly anymore. The implicit annotation approach has it's limit.

But that was the idea, that the compiler complains if someone tries to assign a non-const string to it. Sure, we have to do a cast for internal functions like 'free()' and assignments, but future changes will generate a compiler warning, so the author is at least forced to have a look. I don't see how we could achieve that with any other annotation.

[hreinecke]

(But realistically, the 'const' keyword was merely used to tweak the accessor generator to not generate 'setter' functions. If there's another way of doing that I'm all ears; I'm not wedded to the 'const' thingie.)

[martin-belanger]

But I guess you have to fixup the accessor-generator; while running it when applying the first patch everything's cool, but running it after the second patch it complains about missing setter functions which had been removed by the first patch. @martin-belanger , can you check what is going on?

That's weird. I will have a look.

[igaw]

	nvme_ctrl_lookup_phy_slot(ctx, c->address, (char **)&c->phy_slot);

	check(asprintf((char **)&c.name, "%s_ctrl", test_name) >= 0,
	      "asprintf() failed");

	free((char *)c.name);

is also hiding problems (and looks awful IMO).

As far I know there is no silver bullet for this problem, something like __assign_once would be really cool to have. I think the medicine is worse than the disease in this case.

Also this is about internal library code and I would like to get the API sorted as soon as possible. Another reason why explicit annotation for the generator is my preference. This would decouple this discussion from the setter discussion.

[martin-belanger]

Another reason why explicit annotation for the generator is my preference.

I understand the motivation, but explicit annotations make the parsing side significantly more complex (and it’s already quite challenging even without them). In general, parsing human-written text—with all its variability and edge cases—tends to be fairly brittle.

That said, one possible way to mitigate this would be to move the code generator from C to Python. The original reason for implementing it in C was to allow it to run at build time alongside libnvme, given that not all build environments reliably provide Python (or C++). Since that constraint no longer really applies, switching to Python could make the parsing logic more manageable and flexible.

[hreinecke]

Another reason why explicit annotation for the generator is my preference.

I understand the motivation, but explicit annotations make the parsing side significantly more complex (and it’s already quite challenging even without them). In general, parsing human-written text—with all its variability and edge cases—tends to be fairly brittle.

That said, one possible way to mitigate this would be to move the code generator from C to Python. The original reason for implementing it in C was to allow it to run at build time alongside libnvme, given that not all build environments reliably provide Python (or C++). Since that constraint no longer really applies, switching to Python could make the parsing logic more manageable and flexible.

Or we can make it even easier; only generate 'getter' functions. As my patches demonstrate there are really only a handful of 'setter' functions; most of the attributes cannot be changed once the object is initialized. So why don't we modify the generator to just create 'getter' functions, and create the 'setter' functions by hand?

[hreinecke]

Thanks, that worked quite nicely. I've reworked the patches to use the '!accessors:' annotation syntax, which required not additional code-change. Nice.

[igaw]

Thanks!