805478 - Create a line-based file/socket reader

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dave Hylands [:dhylands]]

Create an async IO Watcher which reads things by lines.

This could be used for files or sockets. Currently, there is duplicated code in the code that reads from netd, and similarly in the code that reads from vold.

This could also be used for reading from things like the /proc/kmsg or any other file/socket based protocol which is line based.

I'm sure that I've got the wrong component for this, but I'm not sure exactly where it belongs.

Comment 1

[StevenLee[:slee]]

Hi Dave,
Do you mean that we can add one class inherits MessageLoopForIO::Watcher? The new class handles the cases that the data is line based. Then we can make NetdClient and VolumeManager to inherit the new class. NetClient and VolumeManager just deal with the line based data. Is that right?

Comment 2

[Dave Hylands [:dhylands]]

Hey Steven,

Yep - that's exactly what I meant.

Comment 3

[StevenLee[:slee]]

(In reply to Dave Hylands [:dhylands] from comment #2)
> Hey Steven,
> 
> Yep - that's exactly what I meant.
Hi Dave,

Could I take this bug?
Thanks.

Comment 4

[Dave Hylands [:dhylands]]

Sure. I've assigned it to you.

I'd be happy to review this for you.

Comment 5

[StevenLee[:slee]]

Attachment: wip patch

Implement LineBaseMessageLoop and change the code in NetdClient.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 677313 [details] [diff] [review]
wip patch

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

This is cetainly a good start. I've suggested different class names, etc.

::: ipc/glue/LineBaseMessageLoop.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

The code from this file should go into ipc/chromium/src/base/message_pump_libevent.cc

@@ +15,5 @@
> +{
> +  ssize_t length = 0;
> +
> +  while (true) {
> +    errno = 0;

I don't think this is needed. We only test errno if read returns -1 and this is the case where read sets errno

@@ +19,5 @@
> +    errno = 0;
> +    length = read(aFd, &mReceiveBuffer, kBufferSize - mReceivedIndex);
> +    MOZ_ASSERT(length <= ssize_t(kBufferSize - mReceivedIndex));
> +    if (length <= 0) {
> +      if (length == -1) {

I'd use < 0

@@ +37,5 @@
> +    }
> +
> +    while (length-- > 0) {
> +      MOZ_ASSERT(mReceivedIndex < kBufferSize);
> +      if (mReceiveBuffer[mReceivedIndex] == '\0') {

Let's make the line-termination character be a parameter in the constructor. That way you can pass in '\n' to read from say a text file.

@@ +38,5 @@
> +
> +    while (length-- > 0) {
> +      MOZ_ASSERT(mReceivedIndex < kBufferSize);
> +      if (mReceiveBuffer[mReceivedIndex] == '\0') {
> +        OnReadLine(aFd, mReceiveBuffer, mReceivedIndex);

Perhaps, we can pass in an nsDependentSubstring rather than a pointer/length. This makes it harder to mess up the data beyond the end of the line

@@ +50,5 @@
> +      }
> +    }
> +  }
> +}
> +

I couldn't find an implementation of  OnFileCanWriteWithoutBlocking.

For VolumeManager/Netd they would provide an implementation, but for say a FileReader, it wouldn't need to.

::: ipc/glue/LineBaseMessageLoop.h
@@ +11,5 @@
> +/**
> + *  LineBaseMessageLoop overrides OnFileCanReadWithoutBlocking. It separates
> + *  the input data by '\0' and callback the parsed data through OnReadLine.
> + */
> +class LineBaseMessageLoop : public MessageLoopForIO::Watcher

I think that you should call the class something like MessageLoopForIO::LineWatcher and put the class definition in ipc/chromium/src/base/message_pump_libevent.h

@@ +14,5 @@
> + */
> +class LineBaseMessageLoop : public MessageLoopForIO::Watcher
> +{
> +public:
> +  LineBaseMessageLoop() : mReceivedIndex(0)

I'd like to see the buffer size passed in as an argument to the constructor, so that users can decide what buffer size to use.

@@ +24,5 @@
> +  /**
> +   * Restart will be called when |read| returns error. Derived class should
> +   * implement this function to handle error cases when needed.
> +   */
> +  virtual void Restart() {}

Perhaps this should be called OnError()

@@ +25,5 @@
> +   * Restart will be called when |read| returns error. Derived class should
> +   * implement this function to handle error cases when needed.
> +   */
> +  virtual void Restart() {}
> +  virtual void OnReadLine(int aFd, char* aBuffer, int aLength) = 0;

I would call this OnLineRead (pronouncing Read as Red, as in past tense of Read (pronounced as Reed))

Comment 7

[StevenLee[:slee]]

Hi Dave,

Thanks for your comments. It makes the patch more readable. I will address them in the next version.
(In reply to Dave Hylands [:dhylands] from comment #6)
> I couldn't find an implementation of  OnFileCanWriteWithoutBlocking.
> 
> For VolumeManager/Netd they would provide an implementation, but for say a
> FileReader, it wouldn't need to.
> 
Sorry for that I am not very clear about this. Would you please give more detail?

Comment 8

[Dave Hylands [:dhylands]]

So your class provides a prototype for OnFileCanWriteWithoutBlocking, but no implementation. 

It makes sense to provide an implementation which asserts when called. This way your class could be used directly by somebody who only needs a reader and doesn't need a reader/writer. The base class declares OnFileCanWriteWithoutBlocking as pure virtual (= 0).

The derived class that needs to be a reader/writer can still override OnFileCanWriteWithoutBlocking and provide a real implementation.

Comment 9

[StevenLee[:slee]]

Attachment: Part 1, implement MessageLoopForIO::LineWatcher

Comment 10

[StevenLee[:slee]]

Attachment: Part2, NetdClient and VolumeManager inherit from LineWatcher

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 678664 [details] [diff] [review]
Part 1, implement MessageLoopForIO::LineWatcher

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

Much better. Still needs some minor rework, but this is pretty close.

::: ipc/chromium/src/base/message_loop.h
@@ +26,5 @@
>  #elif defined(OS_POSIX)
>  #include "base/message_pump_libevent.h"
>  #endif
>  
> +#include "nsDependentSubstring.h"

nit: You didn't seem to add anything in this header which requires nsDependentSubstring.h so you should remove this from here.

::: ipc/chromium/src/base/message_pump_libevent.cc
@@ +391,5 @@
> +      OnError();
> +      mReceivedIndex = 0;
> +      return;
> +    }
> +

Something got dropped here. We also need to check for length == 0, and call OnError in that situation.

So I'd either change the if to check for <= 0 and put the errno checks inside another if (length < 0), or add another if check where this comment is (if length == 0), then you can DLOG an error that suggests that the other end of the pipe was closed.

::: ipc/chromium/src/base/message_pump_libevent.h
@@ +178,5 @@
>  };
>  
> +/**
> + *  LineWatcher overrides OnFileCanReadWithoutBlocking. It separates
> + *  the input data by '\0' and callback the parsed data through OnReadLine.

nit: the comment doesn't match the code (mentions '\0' but the code actually takes a terminator)

@@ +191,5 @@
> +    mReceiveBuffer = new char[mBufferSize];
> +  }
> +
> +  ~LineWatcher() {}
> +private:

virtual methods should probably be protected rather than private. Otherwise the derived class can't override. virtual methods that you're not intending for the derived class to use can be private. So it makes sense for OnFileCanReadWithoutBlocking to be private.

It doesn't make sense for OnError, OnLineRead and OnFileCanWriteWithoutBlocking to be private.

@@ +193,5 @@
> +
> +  ~LineWatcher() {}
> +private:
> +  /**
> +   * Restart will be called when |read| returns error. Derived class should

nit: comment needs to mention OnError instead of Restart

@@ +198,5 @@
> +   * implement this function to handle error cases when needed.
> +   */
> +  virtual void OnError() {}
> +  virtual void OnLineRead(int aFd, nsDependentCSubstring& aMessage) = 0;
> +  virtual void OnFileCanWriteWithoutBlocking(int aFd) {}

nit: I'd either drop aFd, or put (void)aFd inside the function body, otherwise some builds will generate a warning about unused parameter aFd

@@ +204,5 @@
> +
> +  nsAutoPtr<char> mReceiveBuffer;
> +  int mReceivedIndex;
> +  int mBufferSize;
> +  char mTerminateCharacter;

nit: variable names should be named using nouns. So Character is a noun, but Terminate is a verb. So I'd call this mTerminator, or mTerminatorCharacter

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 678665 [details] [diff] [review]
Part2, NetdClient and VolumeManager inherit from LineWatcher

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

r=me with nits fixed and the errno stuff dealt with. I'm happy to re-review if you like.

::: dom/system/gonk/VolumeManager.cpp
@@ +249,5 @@
>    // There are more bytes left to write. Try to write them all.
>    ssize_t bytesWritten = write(mSocket.get(), cmd->Data(), cmd->BytesRemaining());
>    if (bytesWritten < 0) {
>      ERR("Failed to write %d bytes to vold socket", cmd->BytesRemaining());
> +    OnError();

These should continue to call Restart

@@ +279,5 @@
> +  int responseCode = strtol(aMessage.Data(), &endPtr, 10);
> +  if (*endPtr == ' ') {
> +    endPtr++;
> +  }
> +  

nit: remove trailing whitespace

@@ +283,5 @@
> +  
> +  // Now fish out the rest of the line after the response code
> +  nsDependentCString  responseLine(endPtr, aMessage.Length() - (endPtr - aMessage.Data()));
> +  DBG("Rcvd: %d '%s'", responseCode, responseLine.Data());
> +  

nit: remove trailing whitespace

::: dom/system/gonk/VolumeManager.h
@@ -140,5 @@
>    friend class VolumeListCallback; // Calls SetState
>  
>    static void SetState(STATE aNewState);
>  
> -  void Restart();

nit: I'd like to leave this function called Restart. Have OnError call Restart. Restart implies intent, where OnError doesn't.

::: ipc/netd/Netd.cpp
@@ +153,5 @@
> +  // integer response code followed by the rest of the line.
> +  // Fish out the response code.
> +  int responseCode = strtol(aMessage.Data(), nullptr, 10);
> +  // TODO, Bug 783966, handle InterfaceChange(600) and BandwidthControl(601).
> +  if (!errno && responseCode < 600) {

For a successful conversion errno WILL NOT be set by strtol and you'll be using some stale version which may cause this to misbehave.

So the way this if is structured, you need to have the errno = 0 line before calling strtol

And looking at the docs for strtol, they recommend setting errno to 0 before calling it (See the Notes section)
http://linux.die.net/man/3/strtol

@@ +156,5 @@
> +  // TODO, Bug 783966, handle InterfaceChange(600) and BandwidthControl(601).
> +  if (!errno && responseCode < 600) {
> +    NetdCommand* response = new NetdCommand();
> +    // Passing all the response message, including the line terminator.
> +     response->mSize = aMessage.Length();

nit: fix indentation

@@ +164,3 @@
>  
> +   if (!responseCode) {
> +     LOG("Can't parse netd's response: %d (%s)", errno, strerror(errno));

This prints errno, which won't necessarily be the same errno as on line 157 (since calling MessageReceived may call functions which alter errno.

Comment 13

[StevenLee[:slee]]

Attachment: Part 1, implement MessageLoopForIO::LineWatcher - V2

Comment 14

[StevenLee[:slee]]

Attachment: Part 1, implement MessageLoopForIO::LineWatcher - V3

Remove trailing space.

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 679036 [details] [diff] [review]
Part 1, implement MessageLoopForIO::LineWatcher - V3

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

r=me with the final couple of nits fixed.

::: ipc/chromium/src/base/message_pump_libevent.cc
@@ +390,5 @@
> +      } else {
> +        DLOG(ERROR) << "End of file";
> +      }
> +      // At this point, assume that we can't actually access
> +      // the socket anymore, and start a reconnect loop.

nit: replace "and start a reconnect loop." with "and indicate an error."

::: ipc/chromium/src/base/message_pump_libevent.h
@@ +178,5 @@
>  };
>  
> +/**
> + *  LineWatcher overrides OnFileCanReadWithoutBlocking. It separates the input
> + *  data by mTerminator and callback the parsed data through OnReadLine.

nit: OnReadLine s/b OnLineRead.

I'd change this sentence to read

data by mTerminator and passes each line to OnLineRead.

Comment 16

[StevenLee[:slee]]

Attachment: Part 1, implement MessageLoopForIO::LineWatcher - V4

Address comment 15.

Comment 17

[StevenLee[:slee]]

Attachment: Part2, NetdClient and VolumeManager inherit from LineWatcher - V2

Hi Dave,

Please review this patch again.
Thanks.

Comment 18

[StevenLee[:slee]]

Attachment: Part 1, implement MessageLoopForIO::LineWatcher - V5

Fixed the compiling error on Linux and OSX

Comment 19

[StevenLee[:slee]]

Attachment: Part 1, implement MessageLoopForIO::LineWatcher - V5

Replace the wrong patch.
Try server log,
* try: -b d -p linux64 -u all
** https://tbpl.mozilla.org/?tree=Try&rev=34f232042688
* try: -b do -p all -u none
** https://tbpl.mozilla.org/?tree=Try&rev=1b89982e0f6f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2795b0a82775
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f483818db44

Should this have tests?

Comment 21

[Dave Hylands [:dhylands]]

I talked with cjones about this, and he didn't think it was worthwhile to write tests, since this isn't something exposed in the DOM.

As long as higher level users (Volume Manager and Network Manager) continue to work (they have their own tests), then that will be exercising this code.

Comment 22

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2795b0a82775
https://hg.mozilla.org/mozilla-central/rev/9f483818db44