Andy Shevchenko [off-list ref] writes:

On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote:

quoted

On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote:

quoted

On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote:

quoted

Switch to use a generic UUID API instead of custom approach. It
allows to
define UUIDs, compare them, and validate.

[]

Summon initial author of the UUID library.

Summary: the API of comparison functions is rather strange. What the
point to not take pointers directly? (Moreover I hope compiler too
clever not to make a copy of constant arguments there)

I could only imagine the case you are trying to avoid temporary
variables for constants like NULL_UUID.

Issue with this is the ugliness in the users of that, in particularly
present in ACPI (drivers/acpi/apei/ghes.c).

I would like to have more clear interface for that. Perhaps we may add
something like

cmp_p(pointer, non-pointer);
cmp_pp(pointer, pointer);

to not break existing API for now.

It would be useful for many cases in the kernel.

You can take a look at the drivers/acpi/apei/erst.c for uuid_le_cmp
usage.

#define CPER_CREATOR_PSTORE                                             \
        UUID_LE(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, 0x2c,     \
                0x64, 0x90, 0xb8, 0x9d)

        if (uuid_le_cmp(rcd->hdr.creator_id, CPER_CREATOR_PSTORE) != 0)
                goto skip;

Looks better?

This is the typical use case in mind when I write the uuid.h.

As for uuid_le_cmp usage in drivers/acpi/apei/ghes.c,

		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
				 CPER_SEC_PLATFORM_MEM)) {

The code looks not good mainly because acpi_hest_generic_data is not
defined with uuid_le in mind.

struct acpi_hest_generic_data {
	u8 section_type[16];
	u32 error_severity;
	u16 revision;
	u8 validation_bits;
	u8 flags;
	u32 error_data_length;
	u8 fru_id[16];
	u8 fru_text[20];
};

If section_type was defined as uuid_le instead of u8[16], the
uuid_le_cmp usage would look better.  So I suggest to use uuid_le/be in
data structure definition in new code if possible.

Best Regards,
Huang, Ying

quoted

quoted

quoted

+static const uuid_le ads_uuid =
+	UUID_LE(0xdbb8e3e6, 0x5886, 0x4ba6,
+		0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b);
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 					   const union
acpi_object
*desc,

@@ -138,7 +136,7 @@ static bool

acpi_enumerate_nondev_subnodes(acpi_handle scope,
 		    || links->type != ACPI_TYPE_PACKAGE)
 			break;
 
-		if (memcmp(uuid->buffer.pointer, ads_uuid,
sizeof(ads_uuid)))
+		if (uuid_le_cmp(*(uuid_le *)uuid->buffer.pointer,
ads_uuid))

Maybe it's too late, but I don't quite understand the pointer
manipulations here.

I can see why you need a type conversion (although it looks ugly),
but why do you
need to dereference it too?

The function takes that kind of type on input. The other variants are
not compiled.
Perhaps we better change uuid_{lb}e_cmp() first to take normal
pointers, though I think the initial idea was to get type checking at
compile time.