Page MenuHomeFreeBSD

nvmecontrol: Make the error log page work on native format

Authored by imp on Apr 8 2024, 5:00 PM.
Referenced Files
Unknown Object (File)
Mon, Jul 15, 8:21 PM
Unknown Object (File)
Thu, Jun 27, 3:56 PM
Unknown Object (File)
Tue, Jun 25, 6:08 AM
Unknown Object (File)
Sat, Jun 22, 5:55 AM
Unknown Object (File)
Jun 13 2024, 6:27 PM
Unknown Object (File)
May 27 2024, 2:10 AM
Unknown Object (File)
May 22 2024, 4:34 PM
Unknown Object (File)
May 22 2024, 4:34 PM



As the number of page types proliferates, it becomes untennable to
convert them in read_logpage (especailly since new UUID page types will
need to be supported). Convert the error page printing code to operate
on little endian data.

Sponsored by: Netflix

Diff Detail

rG FreeBSD src repository
Lint Not Applicable
Tests Not Applicable

Event Timeline

imp requested review of this revision.Apr 8 2024, 5:00 PM

Where is this defined? Can we not just use 'le32toh' directly instead? In particular, I just use le*dec and le*enc in general throughout the fabrics code. In these cases it's probably fine to use le*toh instead.


No. I hate having to get those right.

So i wrote LE2H with _Generic so i wouldn't have to look them all up. It's in an earlier review in nvmecontrol.h in terms of le*toh


Though i could write a generic to use le*dec too

Also, if I sound kinda short, it was because I'm was typing on my phone for the first answer, which isn't ideal, but it what I did...

268 has the marco:

 * Genericly convert little endian to host endian, based on the type of the thing
 * being converted.
#define LE2H(x)								\
	_Generic(x,							\
	uint8_t: (x),							\
	uint16_t: le16toh(x),						\
	uint32_t: le32toh(x),						\
	uint64_t: le64toh(x))

It's a lot easier to use, will whine if there's something of the wrong type and gets things right. An array, or some other type will fail.


Now, while it is easier to use, we can't expand it to have uint8_t[2] as an alias for uint16_t (array types are never selected in a _Generic statement, they are supposed to match one of the pointer types), so the structs which use arrays, like the self-test page, need an extra copy (I kinda think we should just fix the self-test page to have the right types: the comments are wrong for __packed structs). So there are some limitations to this approach. Plus, using 'x' inside the macro confuses clang sometimes into expanding the wrong thing since it tries to convert pointers to ints, throws a warning it's doing that, and then gets super confused.... I'm not sure I understand that because generic is only supposed to expand one of them...

This revision is now accepted and ready to land.Apr 10 2024, 4:09 AM