Closed Bug 848843 Opened 9 years ago Closed 9 years ago

dmSUT should not call "cat"

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wlach, Assigned: wlach)

References

Details

Attachments

(1 file)

As mcote has pointed out, this method is inherently unsafe, because there is no way to accurately read the number of bytes in the file. The only way to determine whether we're finished reading is to detect the prompt, but we have no way to be sure that the prompt wasn't part of the file.

The "pull" command does everything "cat" does, except safely, since it also transmits the file size. I think we have two options here:

1. Change dmSUT's catFile to just be a wrapper around pullFile. This will preserve backwards compatibility.
2. Remove the catFile method altogether. This will require changes in any client code that uses it, but as far as I can tell nothing does anymore. Even sut_tools (https://hg.mozilla.org/build/tools/file/c290db3dd537/sut_tools/) seems to have migrated off of it.

I prefer (2)
I am fine with #2.

The one exception I know of is in the case of temperature monitoring which I am working on now via:
cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input, pullFile doesn't work for me.  
I also need to adjust the timeouts and stuff, so I wouldn't be using the pullFile function anyway.

I wonder if we can pullFile watcher.ini successfully?
(In reply to Joel Maher (:jmaher) from comment #1)
> I am fine with #2.
> 
> The one exception I know of is in the case of temperature monitoring which I
> am working on now via:
> cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input, pullFile
> doesn't work for me.  
> I also need to adjust the timeouts and stuff, so I wouldn't be using the
> pullFile function anyway.
>
> I wonder if we can pullFile watcher.ini successfully?

To be clear, calling "exec cat" via shellCheckOutput or whatever is still supported. :) It's calling the "cat" command directly which I want to discourage. I assume that's what you're using, and not catFile....

Actually, now that I think about it, since sut_tools does not seem to call the "cat" command, maybe we can outright take it out of the java agent.
I am using:
dm._runCmds([{'cmd': 'cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'}], timeout=1)

This works just fine, but pullFile fails.  I am not using catFile because I needed to limit the timeout.  I would not support pulling cat out of the java agent.
(In reply to Joel Maher (:jmaher) from comment #3)
> I am using:
> dm._runCmds([{'cmd': 'cat
> /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'}], timeout=1)
>
> This works just fine, but pullFile fails.  I am not using catFile because I
> needed to limit the timeout.  I would not support pulling cat out of the
> java agent.

If 'cat' went away, you could just replace this with dm._runCmds([{'cmd': 'exec cat /sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'}], timeout=1). 

Better yet, it is possible to do this without calling internal dmSUT-specific methods by using shellCheckOutput as follows: 

dm.shellCheckOutput(['cat', '/sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input']). 

This has the advantage of not using an internal method which is subject to change, as well as working with DeviceManagerADB. 

Regardless, whether "cat" stays in the agent is not the important thing. I'm more concerned with either kill or modifying the unsafe catFile method inside mozdevice.
I would be very interested in knowing why pull doesn't work with that file.  I'll check it out on my panda.
(In reply to William Lachance (:wlach) from comment #4)
> (In reply to Joel Maher (:jmaher) from comment #3)

> dm.shellCheckOutput(['cat',
> '/sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input']). 

As discussed on irc, that should be dm.shellCheckOutput(['cat', '/sys/bus/platform/devices/temp_sensor_hwmon.0/temp1_input'], timeout=1) if you want the timeout...
I have verified this works and life is all good!
Ok, since we're at least agreed that catFile isn't necessary anymore, let's remove it.
Attachment #722439 - Flags: review?(jmaher)
Comment on attachment 722439 [details] [diff] [review]
Remove catFile method

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

great stuff!
Attachment #722439 - Flags: review?(jmaher) → review+
Pushed: https://github.com/mozilla/mozbase/commit/9edbd3d270ab9e2c0dfba22f101b5afe782de7b0

Let's punt on changing the agent for now. It doesn't seem particularly urgent.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
hmm, I have switched back to my _sendCmd, checkShellOutput is raising exceptions that are not getting caught when I fail to connect.  I will play with this some more when I have time, but I need to keep my machines and tests running.
(In reply to Joel Maher (:jmaher) from comment #11)
> hmm, I have switched back to my _sendCmd, checkShellOutput is raising
> exceptions that are not getting caught when I fail to connect.  I will play
> with this some more when I have time, but I need to keep my machines and
> tests running.

If you could attach a text file demonstrating the problem I'd be interested. shellCheckOutput is supposed to be usable for such things, if it isn't it's a problem. :)
I needed to add
try:
  dm.checkShellOutput(...)
except Exception:
  pass

The key was I needed except Exception.
You need to log in before you can comment on or make changes to this bug.