Andy Shevchenko
2017-May-05 09:24 UTC
[Nouveau] [PATCH v1] ACPI: Switch to use generic UUID API
On Fri, 2017-05-05 at 10:06 +0300, Amir Goldstein wrote:> On Fri, May 5, 2017 at 9:20 AM, Dan Williams <dan.j.williams at intel.com > > wrote: > > On Thu, May 4, 2017 at 2:21 AM, Andy Shevchenko > > <andriy.shevchenko at linux.intel.com> wrote:> > > for (i = 0; i < NFIT_UUID_MAX; i++) > > > - if (memcmp(to_nfit_uuid(i), spa->range_guid, 16) > > > == 0) > > > + if (!uuid_le_cmp_pp(to_nfit_uuid(i), (uuid_le > > > *)spa->range_guid)) > > > > What is _cmp_pp? Why not _compare?Dan, it's a typo. In this patch it should be just ..._cmp(), which is already a part of API.> > > > I second that. > > Andy,Amir, just to be clear. This patch can be applied without any addons to an existing API. Above is just a typo due to rebase in my tree. I will replace it to just uuid_le_cmp().> I much rather that you sort out uuid helpers in a way that will > satisfy the filesystem > needs (just provide the helpers don't need to convert filesystems > code).> The only reason I took a swing at hoisting the xfs uuid helpers is > because it didn't > seem like your proposal was going to be posted soon or wasn't going to > satisfy > the filesystems use case.> > My opinion now, is that your suggestion is probably much closer to the > real deal > than mine. > > IMO, you should acknowledge that the common use case for filesystems > is > to handle an opaque char[16] which most likely holds a uuid_be and you > should provide 'neutral' helpers to satisfy this use case. > > The simplest would be to typedef uuid_t to struct uuid_be and to name > 'neutral' > helpers' uuid_cmp/uuid_copy(uuid_t *, uuid_t *), similar to my > proposal.> I think with this semantic change, our proposals can reach common > grounds > and satisfy a wider group of users (i.e. filesystem developers). > > Christoph also suggested a similar treatment to typedef guid_t to > struct uuid_le. > I don't know the use cases enough to comment on that.We may go this way. But I wouldn't prevent current users of uuid_le to continue using it without conversion (it may be done case by case after we settle an API) So, summarize what Christoph said it will look like typedef uuid_be uuid_t; typedef uuid_le guid_t uuid_cmp() / uuid_copy() / uuid_to_bin() / etc guid_cmp() / guid_copy() / guid_to_bin() / etc Correct? Christoph? -- Andy Shevchenko <andriy.shevchenko at linux.intel.com> Intel Finland Oy
Amir Goldstein
2017-May-05 09:50 UTC
[Nouveau] [PATCH v1] ACPI: Switch to use generic UUID API
On Fri, May 5, 2017 at 12:24 PM, Andy Shevchenko <andriy.shevchenko at linux.intel.com> wrote:> On Fri, 2017-05-05 at 10:06 +0300, Amir Goldstein wrote:[...]>> I think with this semantic change, our proposals can reach common >> grounds >> and satisfy a wider group of users (i.e. filesystem developers). >> >> Christoph also suggested a similar treatment to typedef guid_t to >> struct uuid_le. >> I don't know the use cases enough to comment on that. > > We may go this way. But I wouldn't prevent current users of uuid_le to > continue using it without conversion (it may be done case by case after > we settle an API) > > So, summarize what Christoph said it will look like > > typedef uuid_be uuid_t; > typedef uuid_le guid_t > > uuid_cmp() / uuid_copy() / uuid_to_bin() / etc > guid_cmp() / guid_copy() / guid_to_bin() / etc > > Correct? Christoph? >That looks right to me. To complete the picture for folks not cc'ed on my patches, xfs use case suggests there is also justification for the additional helpers: uuid_is_null() / uuid_equal() guid_is_null() / guid_equal() Cheers, Amir.
Christoph Hellwig
2017-May-05 09:57 UTC
[Nouveau] [PATCH v1] ACPI: Switch to use generic UUID API
On Fri, May 05, 2017 at 12:50:31PM +0300, Amir Goldstein wrote:> To complete the picture for folks not cc'ed on my patches, > xfs use case suggests there is also justification for the additional helpers: > > uuid_is_null() / uuid_equal() > guid_is_null() / guid_equal()The is_null is useful and I bet we'll find other instances. I'm not sure _equals really adds much value over the existing _cmp helpers, but on the other hand they are so trivial that we might as well add them. The other thing XFS has is uuid_copy.