Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> writes: > On Fri, 2016-02-26 at 16:11 +0200, Andy Shevchenko wrote: >> On Thu, 2016-02-18 at 01:03 +0100, Rafael J. Wysocki wrote: >> > >> > On Wednesday, February 17, 2016 02:17:24 PM Andy Shevchenko wrote: >> > > >> > > 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 >> > >> > > >> > > +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. >> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html