Closed Bug 979625 Opened 7 years ago Closed 7 years ago

librecovery can reboot the device without successfully writing the command

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

VERIFIED FIXED
1.4 S3 (14mar)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(1 file)

fwrite is buffered, so we need to check the return value of fclose to ensure that all data was actually flushed. Or just don't use buffered IO.
Requesting blocking since this fix is necessary to actually fix bug 976450 .
Assignee: nobody → mwu
Attachment #8385763 - Flags: review?(mvines)
Comment on attachment 8385763 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/librecovery/pull/7

Turns out fclose is not safe to use to flush things on bionic.
Attachment #8385763 - Flags: review?(mvines)
Comment on attachment 8385763 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/librecovery/pull/7

Made things as paranoid as possible. The linux man pages suggest fsync doesn't return EINTR, while other man pages do. I went with not checking, though if you think that's possible on Linux, I can also add a loop for fsync.
Attachment #8385763 - Flags: review?(dhylands)
Comment on attachment 8385763 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/librecovery/pull/7

Looks good to me.

I'd probably use the TEMP_FAILURE_RETRY macro since its available.
Attachment #8385763 - Flags: review?(dhylands) → review+
I took a look through the kernel sources and all of the waits I found in the fsync code path were of the non-interruptible variety.
(In reply to Dave Hylands [:dhylands] from comment #5)
> I'd probably use the TEMP_FAILURE_RETRY macro since its available.

This macro is pretty nice. I went through and put them in everywhere but safeWrite, where we need a loop to properly write anyway.
Keywords: checkin-needed
blocking-b2g: 1.3? → 1.3+
Master: 1f6a1fe07f81c5bc5e1d079c9b60f7f78ca2bf4f
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
Please request approval-gaia-v1.3 on this patch when this is ready for uplift.
Comment on attachment 8385763 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/librecovery/pull/7

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: People won't get their FOTA updates.
Testing completed: Tested in partition space and inode exhaustion scenarios.
Risk to taking this patch (and alternatives if risky): None. It is perfect.
String or UUID changes made by this patch:
Attachment #8385763 - Flags: approval-mozilla-b2g28?
Naoki, is in the middle of verifying this bug and 976450, hence holding off on approval on these two.
Using the patch and build, the reset phone button does no action as per expected.
I also placed the phone in System Recovery mode by rebooting the phone and then holding the volume up key.
I found that none of the menu items work at all when the cache is filled now.  I am not sure if this is expected or not.

I have to revisit this with the earlier version of the librecovery.so
I found the same issue on an earlier version of the librecovery.so; it's not the patch that is causing it, but the filled cache.  The behavior does change so that we do not give a false impression that the recovery worked.  The phone doesn't reset.

The recovery log in the /cache/recovery/log could not be written to due to being filled
The recovery log in /system/etc/recovery.log was not updated when running this test.

I am not sure if there's a different log file I should check?
Flags: needinfo?(mwu)
/cache/recovery is the only location I'm aware of.

Menu items not working when the cache partition is filled is new though - that should probably be a new bug. I remember the menu items working at MWC on phones that rebooted without a recovery command.
Flags: needinfo?(mwu)
Since the cache is filled, we can't write to the recovery log ( bug 976450 is to help with this issue ); the patch does what it's suppose to do.


Marking as verified.
Gaia      b193de26878dae53a4b20b78e453ed07bfbf4d09
Gecko     cbfe2d6c71daae6ac97b4681c432e950acf16ac6
BuildID   20140310143332
Version   30.0a1
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013
Buri
Status: RESOLVED → VERIFIED
Note: Vendcom bug 982265 filed as I also recall the system recovery options working at MWC on other devices.
Comment on attachment 8385763 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/librecovery/pull/7

Approving given the QA verification. Thanks a lot Naoki for helping with the testing and the set-up here
Attachment #8385763 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
v1.3: 9c4d35857e6a2843aa240e1465aa51780eaf99f6
You need to log in before you can comment on or make changes to this bug.