| Message ID | 1454419402-10769-7-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 02-02-16 om 14:23 schreef Gustavo Padovan: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Making fence_info a pointer enables us to extend the struct in the future > without breaking the ABI. > > v2: use type __u64 for fence_info > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/staging/android/sync.c | 2 +- > drivers/staging/android/uapi/sync.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > index f7530f0..03b1214 100644 > --- a/drivers/staging/android/sync.c > +++ b/drivers/staging/android/sync.c > @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > if (info->status >= 0) > info->status = !info->status; > > - len = sizeof(struct sync_file_info); > + len = sizeof(struct sync_file_info) - sizeof(__u64); > > for (i = 0; i < sync_file->num_fences; ++i) { > struct fence *fence = sync_file->cbs[i].fence; > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > index ed281fc..8e2ed32 100644 > --- a/drivers/staging/android/uapi/sync.h > +++ b/drivers/staging/android/uapi/sync.h > @@ -54,7 +54,7 @@ struct sync_file_info { > char name[32]; > __s32 status; > > - __u8 fence_info[0]; > + __u64 fence_info; > With the information from the commit I would expect fence_info would be a true pointer. so if (sync_file_info->num_fences == 0) not copying fence_info, but set all members including this one. else for_all_fences() copy_to_user(fence_info struct) Otherwise you still can't extend the struct without breaking the abi. ~Maarten
On 2 February 2016 at 15:23, Gustavo Padovan <gustavo@padovan.org> wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Making fence_info a pointer enables us to extend the struct in the future > without breaking the ABI. > > v2: use type __u64 for fence_info > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/staging/android/sync.c | 2 +- > drivers/staging/android/uapi/sync.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > index f7530f0..03b1214 100644 > --- a/drivers/staging/android/sync.c > +++ b/drivers/staging/android/sync.c > @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > if (info->status >= 0) > info->status = !info->status; > > - len = sizeof(struct sync_file_info); > + len = sizeof(struct sync_file_info) - sizeof(__u64); > > for (i = 0; i < sync_file->num_fences; ++i) { > struct fence *fence = sync_file->cbs[i].fence; > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > index ed281fc..8e2ed32 100644 > --- a/drivers/staging/android/uapi/sync.h > +++ b/drivers/staging/android/uapi/sync.h > @@ -54,7 +54,7 @@ struct sync_file_info { > char name[32]; > __s32 status; > > - __u8 fence_info[0]; > + __u64 fence_info; Please update the description in the commit block at the top. As-is things are very confusing :-) -Emil
Hi Maarten, 2016-02-02 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > Op 02-02-16 om 14:23 schreef Gustavo Padovan: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > Making fence_info a pointer enables us to extend the struct in the future > > without breaking the ABI. > > > > v2: use type __u64 for fence_info > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > --- > > drivers/staging/android/sync.c | 2 +- > > drivers/staging/android/uapi/sync.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > > index f7530f0..03b1214 100644 > > --- a/drivers/staging/android/sync.c > > +++ b/drivers/staging/android/sync.c > > @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > > if (info->status >= 0) > > info->status = !info->status; > > > > - len = sizeof(struct sync_file_info); > > + len = sizeof(struct sync_file_info) - sizeof(__u64); > > > > for (i = 0; i < sync_file->num_fences; ++i) { > > struct fence *fence = sync_file->cbs[i].fence; > > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > > index ed281fc..8e2ed32 100644 > > --- a/drivers/staging/android/uapi/sync.h > > +++ b/drivers/staging/android/uapi/sync.h > > @@ -54,7 +54,7 @@ struct sync_file_info { > > char name[32]; > > __s32 status; > > > > - __u8 fence_info[0]; > > + __u64 fence_info; > > > With the information from the commit I would expect fence_info would be a true pointer. I missed that. It is fixed for v3 now. > > so if (sync_file_info->num_fences == 0) not copying fence_info, but set all members including this one. > else for_all_fences() copy_to_user(fence_info struct) > > Otherwise you still can't extend the struct without breaking the abi. all sync_files have at least 1 fence so we are left to the else case, coupy all fences to the user. So we always have variable length at the end of the struct. But I still don't see how this will prevent us from breaking the API. Gustavo
Op 02-02-16 om 21:28 schreef Gustavo Padovan: > Hi Maarten, > > 2016-02-02 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > >> Op 02-02-16 om 14:23 schreef Gustavo Padovan: >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> >>> Making fence_info a pointer enables us to extend the struct in the future >>> without breaking the ABI. >>> >>> v2: use type __u64 for fence_info >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> --- >>> drivers/staging/android/sync.c | 2 +- >>> drivers/staging/android/uapi/sync.h | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c >>> index f7530f0..03b1214 100644 >>> --- a/drivers/staging/android/sync.c >>> +++ b/drivers/staging/android/sync.c >>> @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, >>> if (info->status >= 0) >>> info->status = !info->status; >>> >>> - len = sizeof(struct sync_file_info); >>> + len = sizeof(struct sync_file_info) - sizeof(__u64); >>> >>> for (i = 0; i < sync_file->num_fences; ++i) { >>> struct fence *fence = sync_file->cbs[i].fence; >>> diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h >>> index ed281fc..8e2ed32 100644 >>> --- a/drivers/staging/android/uapi/sync.h >>> +++ b/drivers/staging/android/uapi/sync.h >>> @@ -54,7 +54,7 @@ struct sync_file_info { >>> char name[32]; >>> __s32 status; >>> >>> - __u8 fence_info[0]; >>> + __u64 fence_info; >>> >> With the information from the commit I would expect fence_info would be a true pointer. > I missed that. It is fixed for v3 now. > >> so if (sync_file_info->num_fences == 0) not copying fence_info, but set all members including this one. >> else for_all_fences() copy_to_user(fence_info struct) >> >> Otherwise you still can't extend the struct without breaking the abi. > all sync_files have at least 1 fence so we are left to the else case, > coupy all fences to the user. So we always have variable length at the > end of the struct. But I still don't see how this will prevent us from > breaking the API. Ok this was with fence_info not being a pointer. :)
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index f7530f0..03b1214 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -525,7 +525,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, if (info->status >= 0) info->status = !info->status; - len = sizeof(struct sync_file_info); + len = sizeof(struct sync_file_info) - sizeof(__u64); for (i = 0; i < sync_file->num_fences; ++i) { struct fence *fence = sync_file->cbs[i].fence; diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h index ed281fc..8e2ed32 100644 --- a/drivers/staging/android/uapi/sync.h +++ b/drivers/staging/android/uapi/sync.h @@ -54,7 +54,7 @@ struct sync_file_info { char name[32]; __s32 status; - __u8 fence_info[0]; + __u64 fence_info; }; #define SYNC_IOC_MAGIC '>'