| Message ID | CAB0OVGpFBx9kDE1zdon3X9ecTnoo8XbN4cCwG0_5C+zr=2wORw@mail.gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 06/12/2018 22:37, Carl Eugen Hoyos wrote: > Hi! > > Attached patch fixes building with new Android toolchain, used to be a warning. > > Please comment, Carl Eugen > > From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001 > From: Carl Eugen Hoyos <ceffmpeg@gmail.com> > Date: Thu, 6 Dec 2018 23:34:54 +0100 > Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for ioctl() > on Android. > > Fixes build with new Android toolchain. > --- > libavdevice/v4l2.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c > index 10a0ff0..aa7c052 100644 > --- a/libavdevice/v4l2.c > +++ b/libavdevice/v4l2.c > @@ -95,7 +95,11 @@ struct video_data { > int (*open_f)(const char *file, int oflag, ...); > int (*close_f)(int fd); > int (*dup_f)(int fd); > +#ifdef __ANDROID__ > + int (*ioctl_f)(int fd, int request, ...); > +#else > int (*ioctl_f)(int fd, unsigned long int request, ...); > +#endif > ssize_t (*read_f)(int fd, void *buffer, size_t n); > void *(*mmap_f)(void *start, size_t length, int prot, int flags, int fd, int64_t offset); > int (*munmap_f)(void *_start, size_t length); > -- > 1.7.10.4 > LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for the first case? Looking at possible V4L2-hosting libcs, only glibc has the nonstandard* "unsigned long" rather than "int" as the request argument, so I expect we're going to hit this in more cases (e.g. musl) if compilers are now complaining about it. Thanks, - Mark * POSIX - <http://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html>.
2018-12-09 18:50 GMT+01:00, Mark Thompson <sw@jkqxz.net>: > On 06/12/2018 22:37, Carl Eugen Hoyos wrote: >> Hi! >> >> Attached patch fixes building with new Android toolchain, used to be a >> warning. >> >> Please comment, Carl Eugen >> >> From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001 >> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >> Date: Thu, 6 Dec 2018 23:34:54 +0100 >> Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for >> ioctl() >> on Android. >> >> Fixes build with new Android toolchain. >> --- >> libavdevice/v4l2.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c >> index 10a0ff0..aa7c052 100644 >> --- a/libavdevice/v4l2.c >> +++ b/libavdevice/v4l2.c >> @@ -95,7 +95,11 @@ struct video_data { >> int (*open_f)(const char *file, int oflag, ...); >> int (*close_f)(int fd); >> int (*dup_f)(int fd); >> +#ifdef __ANDROID__ >> + int (*ioctl_f)(int fd, int request, ...); >> +#else >> int (*ioctl_f)(int fd, unsigned long int request, ...); >> +#endif >> ssize_t (*read_f)(int fd, void *buffer, size_t n); >> void *(*mmap_f)(void *start, size_t length, int prot, int flags, int >> fd, int64_t offset); >> int (*munmap_f)(void *_start, size_t length); >> -- >> 1.7.10.4 >> > > LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for > the first case? Looking at possible V4L2-hosting libcs, only glibc has the > nonstandard* "unsigned long" rather than "int" as the request argument, so I > expect we're going to hit this in more cases (e.g. musl) if compilers are > now complaining about it. Is videodev2.h a Linux header as opposed to a c-library header or am I completely misunderstanding? Carl Eugen
On 10/12/2018 00:47, Carl Eugen Hoyos wrote: > 2018-12-09 18:50 GMT+01:00, Mark Thompson <sw@jkqxz.net>: >> On 06/12/2018 22:37, Carl Eugen Hoyos wrote: >>> Hi! >>> >>> Attached patch fixes building with new Android toolchain, used to be a >>> warning. >>> >>> Please comment, Carl Eugen >>> >>> From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001 >>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >>> Date: Thu, 6 Dec 2018 23:34:54 +0100 >>> Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for >>> ioctl() >>> on Android. >>> >>> Fixes build with new Android toolchain. >>> --- >>> libavdevice/v4l2.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c >>> index 10a0ff0..aa7c052 100644 >>> --- a/libavdevice/v4l2.c >>> +++ b/libavdevice/v4l2.c >>> @@ -95,7 +95,11 @@ struct video_data { >>> int (*open_f)(const char *file, int oflag, ...); >>> int (*close_f)(int fd); >>> int (*dup_f)(int fd); >>> +#ifdef __ANDROID__ >>> + int (*ioctl_f)(int fd, int request, ...); >>> +#else >>> int (*ioctl_f)(int fd, unsigned long int request, ...); >>> +#endif >>> ssize_t (*read_f)(int fd, void *buffer, size_t n); >>> void *(*mmap_f)(void *start, size_t length, int prot, int flags, int >>> fd, int64_t offset); >>> int (*munmap_f)(void *_start, size_t length); >>> -- >>> 1.7.10.4 >>> >> >> LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for >> the first case? Looking at possible V4L2-hosting libcs, only glibc has the >> nonstandard* "unsigned long" rather than "int" as the request argument, so I >> expect we're going to hit this in more cases (e.g. musl) if compilers are >> now complaining about it. > > Is videodev2.h a Linux header as opposed to a c-library header or am I > completely misunderstanding? I'm not quite sure what you're asking, so I'll try to explain all of that more carefully: videodev2.h is a Linux header, but it only defines the types and values to call ioctl() with so is mostly orthogonal to this discussion. ioctl() itself is a C library call, but since V4L2 is almost always used with glibc the prototype here has ended up matching that rather than the standard POSIX one. Android follows the POSIX prototype, so your patch is correct. However, so do other standard C libraries such as Musl, and therefore I expect we will hit the same problem with other C libraries in the not-too-distant future. My suggestion, then, is to guard the change with "#ifdef __GLIBC__ <glibc-prototype> #else <posix-prototype> #endif", rather than fixing it only for Android as you have here. Thanks, - Mark
2018-12-10 23:47 GMT+01:00, Mark Thompson <sw@jkqxz.net>: > On 10/12/2018 00:47, Carl Eugen Hoyos wrote: >> 2018-12-09 18:50 GMT+01:00, Mark Thompson <sw@jkqxz.net>: >>> On 06/12/2018 22:37, Carl Eugen Hoyos wrote: >>>> Hi! >>>> >>>> Attached patch fixes building with new Android toolchain, used to be a >>>> warning. >>>> >>>> Please comment, Carl Eugen >>>> >>>> From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001 >>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >>>> Date: Thu, 6 Dec 2018 23:34:54 +0100 >>>> Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for >>>> ioctl() >>>> on Android. >>>> >>>> Fixes build with new Android toolchain. >>>> --- >>>> libavdevice/v4l2.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c >>>> index 10a0ff0..aa7c052 100644 >>>> --- a/libavdevice/v4l2.c >>>> +++ b/libavdevice/v4l2.c >>>> @@ -95,7 +95,11 @@ struct video_data { >>>> int (*open_f)(const char *file, int oflag, ...); >>>> int (*close_f)(int fd); >>>> int (*dup_f)(int fd); >>>> +#ifdef __ANDROID__ >>>> + int (*ioctl_f)(int fd, int request, ...); >>>> +#else >>>> int (*ioctl_f)(int fd, unsigned long int request, ...); >>>> +#endif >>>> ssize_t (*read_f)(int fd, void *buffer, size_t n); >>>> void *(*mmap_f)(void *start, size_t length, int prot, int flags, >>>> int >>>> fd, int64_t offset); >>>> int (*munmap_f)(void *_start, size_t length); >>>> -- >>>> 1.7.10.4 >>>> >>> >>> LGTM on its own, but should this perhaps be "#ifndef (glibc something)" >>> for >>> the first case? Looking at possible V4L2-hosting libcs, only glibc has >>> the >>> nonstandard* "unsigned long" rather than "int" as the request argument, >>> so I >>> expect we're going to hit this in more cases (e.g. musl) if compilers are >>> now complaining about it. >> >> Is videodev2.h a Linux header as opposed to a c-library header or am I >> completely misunderstanding? > > I'm not quite sure what you're asking, so I'll try to explain all of that > more carefully: > > videodev2.h is a Linux header, but it only defines the types and values to > call ioctl() with so is mostly orthogonal to this discussion. I had not thought about this, you are of course right. > ioctl() itself is a C library call, but since V4L2 is almost always used with glibc > the prototype here has ended up matching that rather than the standard POSIX > one. > > Android follows the POSIX prototype, so your patch is correct. However, so > do other standard C libraries such as Musl, and therefore I expect we will > hit the same problem with other C libraries in the not-too-distant future. > My suggestion, then, is to guard the change with "#ifdef __GLIBC__ > <glibc-prototype> #else <posix-prototype> #endif", rather than fixing it > only for Android as you have here. Applied with your suggestion, thank you, Carl Eugen
2018-12-09 18:50 GMT+01:00, Mark Thompson <sw@jkqxz.net>: > On 06/12/2018 22:37, Carl Eugen Hoyos wrote: >> Hi! >> >> Attached patch fixes building with new Android toolchain, used to be a >> warning. >> >> Please comment, Carl Eugen >> >> From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001 >> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >> Date: Thu, 6 Dec 2018 23:34:54 +0100 >> Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for >> ioctl() >> on Android. >> >> Fixes build with new Android toolchain. >> --- >> libavdevice/v4l2.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c >> index 10a0ff0..aa7c052 100644 >> --- a/libavdevice/v4l2.c >> +++ b/libavdevice/v4l2.c >> @@ -95,7 +95,11 @@ struct video_data { >> int (*open_f)(const char *file, int oflag, ...); >> int (*close_f)(int fd); >> int (*dup_f)(int fd); >> +#ifdef __ANDROID__ >> + int (*ioctl_f)(int fd, int request, ...); >> +#else >> int (*ioctl_f)(int fd, unsigned long int request, ...); >> +#endif >> ssize_t (*read_f)(int fd, void *buffer, size_t n); >> void *(*mmap_f)(void *start, size_t length, int prot, int flags, int >> fd, int64_t offset); >> int (*munmap_f)(void *_start, size_t length); >> -- >> 1.7.10.4 >> > > LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for > the first case? Looking at possible V4L2-hosting libcs, only glibc has the > nonstandard* "unsigned long" rather than "int" as the request argument, so I > expect we're going to hit this in more cases (e.g. musl) if compilers are > now complaining about it. OpenBSD also uses the glibc-variant of ioctl(). How should this be fixed? Carl Eugen
From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Thu, 6 Dec 2018 23:34:54 +0100 Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for ioctl() on Android. Fixes build with new Android toolchain. --- libavdevice/v4l2.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c index 10a0ff0..aa7c052 100644 --- a/libavdevice/v4l2.c +++ b/libavdevice/v4l2.c @@ -95,7 +95,11 @@ struct video_data { int (*open_f)(const char *file, int oflag, ...); int (*close_f)(int fd); int (*dup_f)(int fd); +#ifdef __ANDROID__ + int (*ioctl_f)(int fd, int request, ...); +#else int (*ioctl_f)(int fd, unsigned long int request, ...); +#endif ssize_t (*read_f)(int fd, void *buffer, size_t n); void *(*mmap_f)(void *start, size_t length, int prot, int flags, int fd, int64_t offset); int (*munmap_f)(void *_start, size_t length); -- 1.7.10.4