Closed Bug 1091994 Opened 5 years ago Closed 5 years ago

GonkDiskSpaceWatcher is broken on non-ARM due to hard-coded syscall numbers

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

x86
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jld, Assigned: jld)

Details

Attachments

(1 file, 1 obsolete file)

GonkDiskSpaceWatcher has its own syscall stubs for fanotify_init and fanotify_mark (necessary because libc doesn't provide them) which hard-code the syscall numbers for ARM and use them regardless of the actual architecture.

Fortunately, on x86 B2G the number used for fanotify_init is currently not an implemented syscall, so it just fails with ENOSYS and disables GonkDiskSpaceWatcher instead of doing something more confusing than that.

Also fortunately, bug 1086987 means emulator-x86-kk is now completely busted, which allowed this to be noticed, instead of just silently not watching disk space.
Assignee: nobody → jld
Attachment #8514799 - Flags: review?(dhylands)
Component: General → Hardware Abstraction Layer (HAL)
Product: Firefox OS → Core
FYI looking through the bionic sources shows that sys/syscall.h has definitions for __NR_fanotify_init and __NR_fanotify_mark but they've been added only recently:

https://android.googlesource.com/platform/bionic/+/5c2772f59d3b6f564897187324d8606f54423207
(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> https://android.googlesource.com/platform/bionic/+/5c2772f59d3b6f564897187324d8606f54423207

`git branch --contains` suggests this is in JB and up.  Let's see if that's true:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c88ad7713126
Comment on attachment 8514799 [details] [diff] [review]
bug1091994-fanotify-sysnr-hg0.diff

Review of attachment 8514799 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: hal/gonk/GonkDiskSpaceWatcher.cpp
@@ +42,5 @@
> +#elif defined(__i386__)
> +#define __NR_fanotify_init 338
> +#define __NR_fanotify_mark 339
> +#else
> +#error "Unhandled architecture"

Should we add the 64-bit arch as well?

I get 300 and 301 for 64-bit.

Or is this a case where it would already be defined and we don't need the extra defs.

@@ +178,5 @@
>  
>    mFd = fanotify_init(FAN_CLASS_NOTIF, FAN_CLOEXEC);
>    if (mFd == -1) {
> +    int savedErrno = errno;
> +    NS_WARNING("Error calling fanotify_init()");

Couldn't you just move this warning to after the if (for ENOSYS)? Then you don't need to save errno.

If we take the ENOSYS path we'll crash (and there is already a warning in that path).
Attachment #8514799 - Flags: review?(dhylands) → review+
(In reply to Dave Hylands [:dhylands] from comment #4)
> Should we add the 64-bit arch as well?
> 
> I get 300 and 301 for 64-bit.
> 
> Or is this a case where it would already be defined and we don't need the
> extra defs.

The try run indicates that this is, as hoped, fixed in JB.  I don't think we'll ever have x86_64 (or MIPS, or ARM64, or...) on ICS B2G.  I'll also add a comment about that.

> @@ +178,5 @@
> >  
> >    mFd = fanotify_init(FAN_CLASS_NOTIF, FAN_CLOEXEC);
> >    if (mFd == -1) {
> > +    int savedErrno = errno;
> > +    NS_WARNING("Error calling fanotify_init()");
> 
> Couldn't you just move this warning to after the if (for ENOSYS)? Then you
> don't need to save errno.
> 
> If we take the ENOSYS path we'll crash (and there is already a warning in
> that path).

Good point; we probably don't need two warnings in that case.
Carrying over r+.
Attachment #8514799 - Attachment is obsolete: true
Attachment #8515179 - Flags: review+
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6359ce0d02ca is a try run of the first version; the interdiff is trivial, and I've tested this version locally.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7871a7b18a33
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.