Reading data from /sys files occurs quite commonly in Gonk. So we wind up with small helper routines which essentially duplicate functionality, and the duplicated functions are often missing key things (like dealing with EINTR properly) or stripping newlines. We should create a set of helper routines with prototypes something like: bool ReadSysFile(const char *aFilename, char *aBuf, size_t aBufSize); bool ReadSysFile(const char *aFilename, int *aNum); bool ReadSysFile(const char *aFilename, nsACString &aResult) that can be used throughout Gonk. This bug should also hunt out and find those places in the code which are currently using custom local helpers and use the common ones instead.

Something like these: http://hg.mozilla.org/mozilla-central/annotate/287a7d7cf7f0/dom/system/gonk/AutoMounter.cpp#l94 ?

Yeah, although tzimmerman did a couple new versions over here: https://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp#360 These versions have a few improvements. What really needs to be done is mostly refactoring. We're tight for getting B2G released, so I decided to take tzimmerman's changes and file a bug to do the refactoring later.

Hey, this sounds interesting and I'd like to take it up. Who can I talk to about this?

(In reply to Anhad Jai Singh (:ffledgling) from comment #3) > Hey, this sounds interesting and I'd like to take it up. Who can I talk to > about this? Simply post your comments, questions, patches, etc. in this bug report. Regards!

We would take a patch to uplift, but can't block on this.

Is this bug still active? I am fairly new to this platform and would like to help. Could you provide some details on how to proceed? Regards

Yep - this is still active. In order to test this, you'll need a phone that you can run B2G on and you'll need to be able to flash B2G onto it. https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Building_and_installing_Firefox_OS

Here is an example patch to serve as a starting point. Feel free to suggest improvements.

Comment on attachment 723947 [details] [diff] [review] Add two basic functions in FileUtils.cpp and use them in a few places Review of attachment 723947 [details] [diff] [review]: ----------------------------------------------------------------- Normmally, you ask for a review, by putting r? on the patch. You should also change the ownership of the bug. ::: dom/system/gonk/AutoMounter.cpp @@ +113,3 @@ > (strcmp(usbState, "CONFIGURED") == 0); > } > + int configured; I'd really prefer that this stayed as a bool. ::: widget/gonk/nsWindow.cpp @@ +108,4 @@ > > static void *frameBufferWatcher(void *) { > > + char buf[2]; Not sure why you needed to increase the buffer from 1 to 2. The contents aren't used. ::: xpcom/glue/FileUtils.cpp @@ +101,5 @@ > + > +/* XXX use HAL_LOG() for error logging? */ > +#undef ERR > +#define ERR(args...) __android_log_print(ANDROID_LOG_ERROR, \ > + "mozilla::ReadTextFile", ## args) This file has to compile on all platforms. The above will fail on Windows/Mac. If we're going to do any logging at all, we should probably use PR_LOG See: https://developer.mozilla.org/en-US/docs/NSPR_API_Reference/Logging @@ +128,5 @@ > + int fd = TEMP_FAILURE_RETRY(open(aFilename, O_RDONLY)); > + if (fd < 0) { > + int errno_save = errno; > + ERR("Unable to open file '%s' for reading: %s", > + aFilename, strerror(errno)); I don't think that we should have this log. We often have to try different files because different android versions have different /sys files that are read. i.e. It isn't necessarily an error just because a file doesn't exist. Let the caller do a log if its needed. @@ +137,5 @@ > + ssize_t bytesRead = 0; > + size_t offset = 0; > + do { > + bytesRead = TEMP_FAILURE_RETRY(read(fd, aBuf + offset, > + aBufSize - 1 - offset)); To handle the case of passing in 1 byte buffer, don't subtract 1 here. If the caller passes in a single character bufffer we don't want to be trying to read zero bytes. @@ +141,5 @@ > + aBufSize - 1 - offset)); > + if (bytesRead == -1) { > + int errno_save = errno; > + ERR("Unable to read from file '%s': %s", aFilename, strerror(errno)); > + errno = errno_save; Nice touch (restoring errno) @@ +146,5 @@ > + return false; > + } > + offset += bytesRead; > + } while (bytesRead > 0 && offset < aBufSize - 1); > + aBuf[offset] = '\0'; Here, we should make sure we don't go beyond the end of the buffer. This function should always return a null terminated string. Sometimes the content doesn't matter, so it still makes sense to allow for a single character buffer. If the caller really wants to just get back a single character, then a ReadTextFile variant which takes a char * and no length can be introduced (but I don't see any need for that right now). @@ +147,5 @@ > + } > + offset += bytesRead; > + } while (bytesRead > 0 && offset < aBufSize - 1); > + aBuf[offset] = '\0'; > + return true; This function should strip off trailing newlines. Otherwise callers like GonkHal.cpp will fail (since strcmp will be comparing a string with a newline to a string without). @@ +151,5 @@ > + return true; > +} > + > +bool > +mozilla::ParseFile_int( No need for the _int. We can use C++ Polymorphism here and just call all of the variants the same. So this function should have the same name as the previous one: ReadTextFile. Bools and ints are semantically different, so I would prefer to see both variants of the function, rather than changing the caller to use ints. It's perfectly acceptable to have ReadTextFile(bool) call the ReadTextFile(int) variant. ::: xpcom/glue/FileUtils.h @@ +68,5 @@ > + * Read the contents of a file. > + * Read up to "aBufSize - 1" bytes from the given file into the given buffer. > + * The output buffer is always '\0'-terminated. It is assumed that it is a > + * text file and it does not contain '\0' itself. > + * On failure the contents of aBuf after this call is undefined. It should also mention that any trailing newline is removed, and it should probably mention that the intent behind this is for reading single-line /sys files. I still think that the function and its overloads should be called ReadSysFile as this isn't really intended for reading general purpose Text files. We should also probably put a #ifdef MOZ_WIDGET_GONK around the functions so we aren't increasing the size of libxul for non-gonk platforms. All of the callers are currently gonk.

(In reply to Dave Hylands [:dhylands] from comment #9) [...] > Review of attachment 723947 [details] [diff] [review]: [...] > You should also change the ownership of the bug. How do I do that? > ::: dom/system/gonk/AutoMounter.cpp > @@ +113,3 @@ > > (strcmp(usbState, "CONFIGURED") == 0); > > } > > + int configured; > > I'd really prefer that this stayed as a bool. Done. > ::: widget/gonk/nsWindow.cpp > @@ +108,4 @@ > > > > static void *frameBufferWatcher(void *) { > > > > + char buf[2]; > > Not sure why you needed to increase the buffer from 1 to 2. The contents > aren't used. Because ReadSysFile() leaves 1 byte for the terminating '\0' and it will end up with a request to read(2) zero bytes if a 1-byte buffer is passed, but I have changed that, see below. > ::: xpcom/glue/FileUtils.cpp > @@ +101,5 @@ > > + > > +/* XXX use HAL_LOG() for error logging? */ > > +#undef ERR > > +#define ERR(args...) __android_log_print(ANDROID_LOG_ERROR, \ > > + "mozilla::ReadTextFile", ## args) > > This file has to compile on all platforms. The above will fail on > Windows/Mac. > > If we're going to do any logging at all, we should probably use PR_LOG > See: https://developer.mozilla.org/en-US/docs/NSPR_API_Reference/Logging Ok, removed the logging from ReadSysFile(). errno will always be set after ReadSysFile() has failed, so the caller can take care. The only inconvenience is that the caller will not know which one of open(2) or read(2) failed. Notice that now all the logging is completely removed. I should probably add logging of the errors to some of the callers of ReadSysFile(). > @@ +128,5 @@ > > + int fd = TEMP_FAILURE_RETRY(open(aFilename, O_RDONLY)); > > + if (fd < 0) { > > + int errno_save = errno; > > + ERR("Unable to open file '%s' for reading: %s", > > + aFilename, strerror(errno)); > > I don't think that we should have this log. We often have to try different > files because different android versions have different /sys files that are > read. > > i.e. It isn't necessarily an error just because a file doesn't exist. Let > the caller do a log if its needed. Done. > @@ +137,5 @@ > > + ssize_t bytesRead = 0; > > + size_t offset = 0; > > + do { > > + bytesRead = TEMP_FAILURE_RETRY(read(fd, aBuf + offset, > > + aBufSize - 1 - offset)); > > To handle the case of passing in 1 byte buffer, don't subtract 1 here. If > the caller passes in a single character bufffer we don't want to be trying > to read zero bytes. [...] Done. > @@ +146,5 @@ > > + return false; > > + } > > + offset += bytesRead; > > + } while (bytesRead > 0 && offset < aBufSize - 1); > > + aBuf[offset] = '\0'; > > Here, we should make sure we don't go beyond the end of the buffer. This > function should always return a null terminated string. The above code does this. > Sometimes the content doesn't matter, so it still makes sense to allow for a > single character buffer. > > If the caller really wants to just get back a single character, then a > ReadTextFile variant which takes a char * and no length can be introduced > (but I don't see any need for that right now). Ok, I changed it to handle aBufSize==1 and not request to read(2) zero bytes. Now the function returns an empty string in that case. > @@ +147,5 @@ > > + } > > + offset += bytesRead; > > + } while (bytesRead > 0 && offset < aBufSize - 1); > > + aBuf[offset] = '\0'; > > + return true; > > This function should strip off trailing newlines. Otherwise callers like > GonkHal.cpp will fail (since strcmp will be comparing a string with a > newline to a string without). Ah, I intended to change that to use strncmp() and thus only compare whether the file contents starts with the given string. The idea was to make the ReadSysFile() function as generic as possible (not modifying the file contents in any way, like wiping away the new lines). Now I have put a proper whitespace removal. Let me know if the strncmp() idea seems more plausible to you. > @@ +151,5 @@ > > + return true; > > +} > > + > > +bool > > +mozilla::ParseFile_int( > > No need for the _int. We can use C++ Polymorphism here and just call all of > the variants the same. > > So this function should have the same name as the previous one: ReadTextFile. Done. > Bools and ints are semantically different, so I would prefer to see both > variants of the function, rather than changing the caller to use ints. It's > perfectly acceptable to have ReadTextFile(bool) call the ReadTextFile(int) > variant. Done. > ::: xpcom/glue/FileUtils.h > @@ +68,5 @@ > > + * Read the contents of a file. > > + * Read up to "aBufSize - 1" bytes from the given file into the given buffer. > > + * The output buffer is always '\0'-terminated. It is assumed that it is a > > + * text file and it does not contain '\0' itself. > > + * On failure the contents of aBuf after this call is undefined. > > It should also mention that any trailing newline is removed, and it should > probably mention that the intent behind this is for reading single-line /sys > files. Done. > I still think that the function and its overloads should be called > ReadSysFile as this isn't really intended for reading general purpose Text > files. Done. > We should also probably put a #ifdef MOZ_WIDGET_GONK around the functions so > we aren't increasing the size of libxul for non-gonk platforms. All of the > callers are currently gonk. Done. Notice that files in /sys/ are also read in the following two locations in B2G/gecko/: nsprpub/pr/src/misc/prsystem.c:238 ReadSysFile() could be used here, but this is C source, some tweaks will be needed. b2g/app/BootAnimation.cpp:560 ReadSysFile() should be used here, but I get undefined reference to it during link time from BootAnimation.o, still not investigated. (and some others too, but ReadSysFile() is not applicable there)

Update to the previous patch, addressing Dave's suggestions.

"Fix" an edge case where if the file contains abc\nd and the buffer size is 5, then the result from the function will be abc\n which has trailing whitespace.

I'm just replying to the comment, I'll wait until you repost the code before reviewing. (In reply to Vasil Dimov [:vd] from comment #10) > (In reply to Dave Hylands [:dhylands] from comment #9) > [...] > > Review of attachment 723947 [details] [diff] [review]: > [...] > > You should also change the ownership of the bug. > > How do I do that? At the top of the bug where it says "Assigned To:" nobody, click on take. If you don't have the ability to do that (I seem to recall that you may need some bugzilla privlege to do that, we can get you the ability to do that). > Notice that now all the logging is completely removed. I should > probably add logging of the errors to some of the callers of > ReadSysFile(). We'd need to look at those on a case by case basis to decide if its worthwhile or not. > > @@ +147,5 @@ > > > + } > > > + offset += bytesRead; > > > + } while (bytesRead > 0 && offset < aBufSize - 1); > > > + aBuf[offset] = '\0'; > > > + return true; > > > > This function should strip off trailing newlines. Otherwise callers like > > GonkHal.cpp will fail (since strcmp will be comparing a string with a > > newline to a string without). > > Ah, I intended to change that to use strncmp() and thus only compare > whether the file contents starts with the given string. The idea was to > make the ReadSysFile() function as generic as possible (not modifying > the file contents in any way, like wiping away the new lines). > > Now I have put a proper whitespace removal. Let me know if the strncmp() > idea seems more plausible to you. Personally, I'd rather not. Having the newline be there is subtle enough that others have written buggy code because they didn't think to strip it/compensate for it. I only think that a trailing newline should be stripped and not anything else. If the sys file has multiple lines of output, I'd still only strip the trailing newline at the very end, and not anything embedded. Most of the sys files that we're dealing with are single line, so stripping the trailing newline allows us to use strcmp (which is what people seem to use by default). > Done. Notice that files in /sys/ are also read in the following two > locations in B2G/gecko/: > > nsprpub/pr/src/misc/prsystem.c:238 > ReadSysFile() could be used here, but this is C source, some tweaks > will be needed. nspr is special. It can only call functions contained within its own tree (it's really a standalone package). So we'd have to put ReadSysFile into that in order to use it. So I think its fine to just ignore that for now. > b2g/app/BootAnimation.cpp:560 > ReadSysFile() should be used here, but I get undefined reference to it > during link time from BootAnimation.o, still not investigated. I think that the BootAnimation may be a standalone program that doesn't link in libxul.

Update to the previous patch: * remove only a single trailing newline, as suggested by Dave * Add error logging in case of failures to the places where ReadSysFile() failed if we have already checked that the file exists

Comment on attachment 724958 [details] [diff] [review] Add the functions in FileUtils.cpp and use them in a few places Review of attachment 724958 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Just a couple more cleanups and this should be good to go. ::: xpcom/glue/FileUtils.cpp @@ +103,5 @@ > + > +#undef TEMP_FAILURE_RETRY > +/* XXX EAGAIN, EMFILE, ENFILE, ENOMEM, ENOSPC, ETXTBSY, EWOULDBLOCK > +could be transient errors too - it makes sense to retry a few times if > +one of them occurs before giving up. */ EAGAIN and EWOULDBLOCK would only occur if the file was opened non-blocking. There are definitely cases where you wouldn't want to retry (i.e. you're reading a pipe/socket and no data is available). ENOMEM won't happen (the process will be killed by the OOM killer first) ENOSPC may or may not be transient, depending on the volume. ETXTBSY should only happen if you're trying to write to an executable file, which we're not doing here. That leaves EMFILE and ENFILE. I think it really depends on why these errors are occuring, and we probably shouldn't retry. So I'd be inclined to remove the comment, since most of it doesn't apply. EINTR is the realworld thing that often happens and people forgot to code for it, which is why we're checking here. @@ +139,5 @@ > + MOZ_ASSERT(offset <= aBufSize); > + if (offset == aBufSize) { > + MOZ_ASSERT(offset > 0); > + offset--; > + } This test should go after the newline test. Then the code is doing: 1 - strip trailing newline 2 - forcibly null terminate the resulting string. Right now part of step 2 happens before step 1 and part happens after. So if I read: abcd\n\n and passed in a buffer of 6, both newlines would be stripped. @@ +168,5 @@ > + int v; > + if (!ReadSysFile(aFilename, &v)) { > + return false; > + } > + return v != 0; Shouldn't this be: *aVal = (v != 0); return true;

(In reply to Dave Hylands [:dhylands] from comment #15) [...] > Looks good. Just a couple more cleanups and this should be good to go. > > ::: xpcom/glue/FileUtils.cpp > @@ +103,5 @@ > > + > > +#undef TEMP_FAILURE_RETRY > > +/* XXX EAGAIN, EMFILE, ENFILE, ENOMEM, ENOSPC, ETXTBSY, EWOULDBLOCK > > +could be transient errors too - it makes sense to retry a few times if > > +one of them occurs before giving up. */ [...] > So I'd be inclined to remove the comment, since most of it doesn't apply. > EINTR is the realworld thing that often happens and people forgot to code > for it, which is why we're checking here. Done. > @@ +139,5 @@ > > + MOZ_ASSERT(offset <= aBufSize); > > + if (offset == aBufSize) { > > + MOZ_ASSERT(offset > 0); > > + offset--; > > + } > > This test should go after the newline test. Then the code is doing: > > 1 - strip trailing newline > 2 - forcibly null terminate the resulting string. > > Right now part of step 2 happens before step 1 and part happens after. So if > I read: > > abcd\n\n and passed in a buffer of 6, both newlines would be stripped. Done. Is there a place where I could plant some unit tests calling this function with a bunch of edge cases to ensure it behaves as expected, especially after changes in the future? Right now I did test the (abcd\n\n, 6) case and assumed that the behavior is fine. In order to test it I did copy-paste the function into a separate file and compiled it standalone (luckily ReadSysFile() did not depend on other parts of gecko except the ScopedClose thing, which I just deleted from the test program). > @@ +168,5 @@ > > + int v; > > + if (!ReadSysFile(aFilename, &v)) { > > + return false; > > + } > > + return v != 0; > > Shouldn't this be: > > *aVal = (v != 0); > return true; Of course it should be, good catch! :)

(In reply to Vasil Dimov [:vd] from comment #16) > Is there a place where I could plant some unit tests calling this function > with a bunch of edge cases to ensure it behaves as expected, especially > after changes in the future? I've asked the same question, and was told that adding C++ tests is "hard", whatever that means, so I haven't personally persued it any further.

Comment on attachment 725307 [details] [diff] [review] Add the functions in FileUtils.cpp and use them in a few places This looks good to me. Since it affects FileUtils.cpp, (xpcom/glue), I'm going to ask bsmedberg for a review (he's down as the module owner for xpcom/glue)

Comment on attachment 725307 [details] [diff] [review] Add the functions in FileUtils.cpp and use them in a few places Asking bsmedberg to verify that this looks good, since FileUtils.cpp is in xpcom/glue. I asked vd to put #ifdef MOZ_WIDGET_GONK around the ReadSysFile functions since they're currently only being called from gonk, and I didn't want to increase libxul.so size for non-gonk platforms.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b80fc342465 Should this have tests?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21) > https://hg.mozilla.org/integration/mozilla-inbound/rev/4b80fc342465 > > Should this have tests? It could have. They would have to be C++ tests. Every time I've asked about this I was told that they were "hard". Are there any decent examples around?

I remember that bent got some C++ tests running recently.

If there isn't a deployed C++ unit tests framework, what about planting one? Has anybody played with e.g. google test [1]? [1] http://code.google.com/p/googletest/

We added support for Google Test-based C++ tests in bug 767231.

(In reply to Vasil Dimov [:vd] from comment #24) > If there isn't a deployed C++ unit tests framework Just found this file: ipc/chromium/src/base/process_util_unittest.cc, looks like some parts of the codebase are already using gtest.

The Mozilla gtest doc is here: https://developer.mozilla.org/en-US/docs/GTest, but it somehow did not describe well what needed to be done in the attached patch. Maybe it needs to be updated?

Comment on attachment 736359 [details] [diff] [review] Unit tests for the newly added functions, using gtest Review of attachment 736359 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/gtest/TestFileUtils.cpp @@ +78,5 @@ > + > +TEST(ReadSysFile, Main) { > + /* Use a different file name for each test since different tests could be > + executed concurrently. */ > + static const char* fn = "TestReadSysFileMain"; Consider this - the temporary file will be created in the current working directory. Eventually, it may not be writeable. Probably would be better to use mkstemp(3) and /tmp or $TMP or $TMPDIR from the environment. If this is going to be executed on Windows, then _mktemp_s() could be used there.

I opened a followup bug to track the unit tests: Bug 860827

Comment on attachment 736359 [details] [diff] [review] Unit tests for the newly added functions, using gtest Review of attachment 736359 [details] [diff] [review]: ----------------------------------------------------------------- I'm not familiar with the function being tested. I hope dhylands can review that. r- because I'd like to look at the corrections. ::: xpcom/glue/FileUtils.cpp @@ +117,4 @@ > return false; > } > > +#if defined(MOZ_WIDGET_GONK) || defined(MOZ_ENABLE_GTEST) Using MOZ_ENABLE_GTEST isn't appropriate here (and this define is temporary and will go away soon). You should instead only have a unit test for platforms where this method is compiled. @@ +188,4 @@ > return true; > } > > +#endif /* MOZ_WIDGET_GONK || MOZ_ENABLE_GTEST */ same ::: xpcom/glue/FileUtils.h @@ +145,4 @@ > const size_t aCount = SIZE_MAX); > > > +#if defined(MOZ_WIDGET_GONK) || defined(MOZ_ENABLE_GTEST) same ::: xpcom/glue/gtest/Makefile.in @@ +1,4 @@ > +# vim:set ts=8 sw=8 sts=8 noet: > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. Can you move xpcom/glue/gtest to xpcom/glue/tests/gtest. Sorry this was just decided: https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/hmUzuq7P31o ::: xpcom/glue/gtest/TestFileUtils.cpp @@ +17,5 @@ > +#include "FileUtils.h" > + > +#include "gtest/gtest.h" > + > +#ifndef MOZ_ENABLE_GTEST MOZ_ENABLE_GTEST is going to go away, we shouldn't use it here. @@ +29,5 @@ > +/** > + * Create a file with the specified contents. > + */ > +bool > +WriteFile( static

A patch that addresses the above was posted to Bug 860827