| Message ID | 20170505075725.19597-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, May 5, 2017 at 10:57 AM, Christoph Hellwig <hch@lst.de> wrote: > Use the generic Linux definition to implement our UUID type, this will > allow using more generic infrastructure in the future. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Amir Goldstein <amir73il@gmail.com> > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h > index 044fb0e15390..89ee5ec66837 100644 > --- a/fs/xfs/xfs_linux.h > +++ b/fs/xfs/xfs_linux.h > @@ -19,6 +19,7 @@ > #define __XFS_LINUX__ > > #include <linux/types.h> > +#include <linux/uuid.h> > > /* > * Kernel specific type declarations for XFS > @@ -38,6 +39,8 @@ typedef __s64 xfs_daddr_t; /* <disk address> type */ > typedef __u32 xfs_dev_t; > typedef __u32 xfs_nlink_t; > > +typedef uuid_be uuid_t; > + How about moving this typedef to linux/uuid.h. It's weird to have a non xfs_ prefixed typedef as it was and placing it here is even more weird. Yes, only xfs uses uuid_t right now, but this could mark the intentions in a central place, so other code can follow suit (i.e. libnvdimm) and start using uuid_t as well. I realize this series is intended to be xfs internal cleanup, so could be part of a follow up patch set. If this is acceptable by Andy, I can re-post my series based on top of this one to hoist uuid_t and the rest of the xfs helpers to linux/uuid.h. Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 05, 2017 at 11:44:16AM +0300, Amir Goldstein wrote: > How about moving this typedef to linux/uuid.h. Yes, I think eventually we want that, but for now I tried to keep it local to XFS. uuid.h already is a mess with uuid_be/uuid_le and struct uuid_v1. I think I'll need to do a series on that first, but this might run into conflicts with the work that Andy is doing at the moment. > It's weird to have a non xfs_ prefixed typedef as it was > and placing it here is even more weird. We do have a few typedefs like that, but maybe we should eventually clean them up. > Yes, only xfs uses uuid_t right now, but this could mark the intentions > in a central place, so other code can follow suit (i.e. libnvdimm) and > start using uuid_t as well. I think libnvdimm would be guid_t. > If this is acceptable by Andy, I can re-post my series based on top > of this one to hoist uuid_t and the rest of the xfs helpers to linux/uuid.h. Sure. There actually are very few users of uuid_be at the moment, so it might be a good opportunity to just kill if off ASAP. uuid_le might take a little more time if it's really worth it. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-05-05 at 10:48 +0200, Christoph Hellwig wrote: > On Fri, May 05, 2017 at 11:44:16AM +0300, Amir Goldstein wrote: > > How about moving this typedef to linux/uuid.h. > > Yes, I think eventually we want that, but for now I tried to keep > it local to XFS. uuid.h already is a mess with uuid_be/uuid_le and > struct uuid_v1. > > I think I'll need to do a series on that first, but this might > run into conflicts with the work that Andy is doing at the moment. Christoph, I can wait a bit and re-do my patch if we settle down data types and function name space. I would really get rid of the mess with UUIDs/GUIDs we have in kernel (but I'm not FS expert, so, I didn't touch that area at all). > > It's weird to have a non xfs_ prefixed typedef as it was > > and placing it here is even more weird. > > We do have a few typedefs like that, but maybe we should eventually > clean them up. > > > Yes, only xfs uses uuid_t right now, but this could mark the > > intentions > > in a central place, so other code can follow suit (i.e. libnvdimm) > > and > > start using uuid_t as well. > > I think libnvdimm would be guid_t. Yes. EFI, ACPI, this one are the users of uuid_le. AFAIR EFI defines type as efi_guid_t. > > > If this is acceptable by Andy, I can re-post my series based on top > > of this one to hoist uuid_t and the rest of the xfs helpers to > > linux/uuid.h. > > Sure. There actually are very few users of uuid_be at the moment, > so it might be a good opportunity to just kill if off ASAP. Agreed. > uuid_le might take a little more time if it's really worth it. We may leave the type and helpers, introduce better ones and encourage new code to use them instead.
On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote: > Christoph, I can wait a bit and re-do my patch if we settle down data > types and function name space. We should probably just work on a common branch. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-05-05 at 11:56 +0200, Christoph Hellwig wrote: > On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote: > > Christoph, I can wait a bit and re-do my patch if we settle down > > data > > types and function name space. > > We should probably just work on a common branch. Works for me. Are you volunteering to provide a such?
On Fri, May 05, 2017 at 01:06:28PM +0300, Andy Shevchenko wrote: > On Fri, 2017-05-05 at 11:56 +0200, Christoph Hellwig wrote: > > On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote: > > > Christoph, I can wait a bit and re-do my patch if we settle down > > > data > > > types and function name space. > > > > We should probably just work on a common branch. > > Works for me. Are you volunteering to provide a such? Sure, I can set one up in one of my infradead trees. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ added CC linux-fsdevel ] On Fri, May 5, 2017 at 1:10 PM, Christoph Hellwig <hch@lst.de> wrote: > On Fri, May 05, 2017 at 01:06:28PM +0300, Andy Shevchenko wrote: >> On Fri, 2017-05-05 at 11:56 +0200, Christoph Hellwig wrote: >> > On Fri, May 05, 2017 at 12:42:23PM +0300, Andy Shevchenko wrote: >> > > Christoph, I can wait a bit and re-do my patch if we settle down >> > > data >> > > types and function name space. >> > >> > We should probably just work on a common branch. >> >> Works for me. Are you volunteering to provide a such? > > Sure, I can set one up in one of my infradead trees. Christoph, Andy, Our discussions now span so many threads and different CC lists (ACPI uuid_le, hoist xfs uuid_t, tmpfs gen s_uuid, ubifs copy s_uuid, ...) so I picked this relatively young and less overloaded thread. Since converting filesystems and uuid_be seems to be first order of business, I only added linux-fsdevel to CC. If you think that other lists should be included at this point in the discussion, feel free to add them. Let's try to stick to this thread going forward. To avoid stepping over each other toes, I propose the following work sequence: This patch set by Christoph: - re-define xfs's uuid_t as uuid_be (this patch) - more xfs uuid cleanups V3 of 'hoist xfs uuid helpers' by me: - hoist uuid_t typedef to uuid.h - hoist helpers uuid_equal()/uuid_copy() to uuid.h - create helper uuid_is_null() - convert xfs to use the new uuid_is_null() Kill off uuid_be (I can do it): - Create helper variants uuid_cmp()/uuid_gen()/uuid_to_bin() - Convery uuid_be users to uuid_t and uuid_ helpers - Kill off uuid_be and uuid_be_ helpers Promote uuid_t to VFS and filesystems (I can do it): - Convert sb->s_uuid to uuid_t - Convert some sample fs (tmpfs, ext2, ext4, ..) to uuid_t - Use uuid_copy() and uuid_gen() where due Kill off generate_random_uuid (Christoph?): - Not sure what you're intentions are wrt to all users that use this helper to initialize a char[16] var? Maybe repeat the uuid_be->uuid_t process with uuid_le->guid_t (Andy?): - I don't think any of the above interferes with your work to convert ACPI to uuid_le? - You can actually start this work in parallel to uuid_be->uuid_t, because it will create trivial merge conflicts that we can easily sort out on the common development branch Thoughts? Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 10, 2017 at 11:39:27AM +0300, Amir Goldstein wrote: > To avoid stepping over each other toes, I propose the following work sequence: I agree on the list, but I've started a tree that has a slightly different sequence: (1) renamed uuid_be to uuid_t and uuid_le to guid_t, but keep the old name for compat for now. Including these items from your list: > - Create helper variants uuid_cmp()/uuid_gen()/uuid_to_bin() (2): kill uuid_v1 by adding a union to uuid_t that has the v1 meaning (and maybe more in the future) (3) > This patch set by Christoph: > - re-define xfs's uuid_t as uuid_be (this patch) > - more xfs uuid cleanups (4): > V3 of 'hoist xfs uuid helpers' by me: > - hoist uuid_t typedef to uuid.h > - hoist helpers uuid_equal()/uuid_copy() to uuid.h > - create helper uuid_is_null() > - convert xfs to use the new uuid_is_null() (but now moved over the to plain uuid_t type) (5) > Promote uuid_t to VFS and filesystems (I can do it): > - Convert sb->s_uuid to uuid_t > - Convert some sample fs (tmpfs, ext2, ext4, ..) to uuid_t > - Use uuid_copy() and uuid_gen() where due And the rest for latter > Kill off uuid_be (I can do it): > - Convery uuid_be users to uuid_t and uuid_ helpers > - Kill off uuid_be and uuid_be_ helpers > > Kill off generate_random_uuid (Christoph?): > - Not sure what you're intentions are wrt to all users that use > this helper to initialize a char[16] var? They really should be converted to either uuid_t or guid_t, or just use prandom_bytes if they really aren't either a UUID or GUID > Maybe repeat the uuid_be->uuid_t process with uuid_le->guid_t (Andy?): > - I don't think any of the above interferes with your work to convert > ACPI to uuid_le? > - You can actually start this work in parallel to uuid_be->uuid_t, because > it will create trivial merge conflicts that we can easily sort out > on the common > development branch With my work I'm right now at item (5) of my list, I'll send it out later this afternoon. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-05-10 at 14:01 +0200, Christoph Hellwig wrote: > On Wed, May 10, 2017 at 11:39:27AM +0300, Amir Goldstein wrote: > > To avoid stepping over each other toes, I propose the following work > > sequence: > > I agree on the list, but I've started a tree that has a slightly > different > sequence: > > (1) renamed uuid_be to uuid_t and uuid_le to guid_t, but keep the old > name for compat for now. > Including these items from your list: > > - Create helper variants uuid_cmp()/uuid_gen()/uuid_to_bin() > > (2): kill uuid_v1 by adding a union to uuid_t that has the v1 meaning > (and maybe more in the future) > > (3) > This patch set by Christoph: > > - re-define xfs's uuid_t as uuid_be (this patch) > > - more xfs uuid cleanups > > > (4): > V3 of 'hoist xfs uuid helpers' by me: > > - hoist uuid_t typedef to uuid.h > > - hoist helpers uuid_equal()/uuid_copy() to uuid.h > > - create helper uuid_is_null() > > - convert xfs to use the new uuid_is_null() > > (but now moved over the to plain uuid_t type) > > (5) > Promote uuid_t to VFS and filesystems (I can do it): > > - Convert sb->s_uuid to uuid_t > > - Convert some sample fs (tmpfs, ext2, ext4, ..) to uuid_t > > - Use uuid_copy() and uuid_gen() where due > > And the rest for latter > > > Kill off uuid_be (I can do it): > > - Convery uuid_be users to uuid_t and uuid_ helpers > > - Kill off uuid_be and uuid_be_ helpers > > > > Kill off generate_random_uuid (Christoph?): > > - Not sure what you're intentions are wrt to all users that use > > this helper to initialize a char[16] var? > > They really should be converted to either uuid_t or guid_t, or > just use prandom_bytes if they really aren't either a UUID or GUID > > > Maybe repeat the uuid_be->uuid_t process with uuid_le->guid_t > > (Andy?): > > - I don't think any of the above interferes with your work to > > convert > > ACPI to uuid_le? > > - You can actually start this work in parallel to uuid_be->uuid_t, > > because > > it will create trivial merge conflicts that we can easily sort out > > on the common > > development branch > > With my work I'm right now at item (5) of my list, I'll send it out > later this afternoon. I'm fine with above and it looks like I could rebase my current work whenever (1) has been done. Looking forward to repository with changes I can rebase on top of.
On Wed, May 10, 2017 at 03:54:55PM +0300, Andy Shevchenko wrote: > I'm fine with above and it looks like I could rebase my current work > whenever (1) has been done. > > Looking forward to repository with changes I can rebase on top of. Here is what I currently have: http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/uuid-types It hasn't survived the buildbot yet, so take it with a grain of salt. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/uuid.h b/fs/xfs/uuid.h index 104db0f3bed6..4f1441ba4fa5 100644 --- a/fs/xfs/uuid.h +++ b/fs/xfs/uuid.h @@ -18,10 +18,6 @@ #ifndef __XFS_SUPPORT_UUID_H__ #define __XFS_SUPPORT_UUID_H__ -typedef struct { - unsigned char __u_bits[16]; -} uuid_t; - extern int uuid_is_nil(uuid_t *uuid); extern int uuid_equal(uuid_t *uuid1, uuid_t *uuid2); extern void uuid_getnodeuniq(uuid_t *uuid, int fsid [2]); diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h index 044fb0e15390..89ee5ec66837 100644 --- a/fs/xfs/xfs_linux.h +++ b/fs/xfs/xfs_linux.h @@ -19,6 +19,7 @@ #define __XFS_LINUX__ #include <linux/types.h> +#include <linux/uuid.h> /* * Kernel specific type declarations for XFS @@ -38,6 +39,8 @@ typedef __s64 xfs_daddr_t; /* <disk address> type */ typedef __u32 xfs_dev_t; typedef __u32 xfs_nlink_t; +typedef uuid_be uuid_t; + #include "xfs_types.h" #include "kmem.h"
Use the generic Linux definition to implement our UUID type, this will allow using more generic infrastructure in the future. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/uuid.h | 4 ---- fs/xfs/xfs_linux.h | 3 +++ 2 files changed, 3 insertions(+), 4 deletions(-)