[Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243)

["Toke Høiland-Jørgensen" <notifications@github.com>]

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

@tohojo requested changes on this pull request.

Please add a paragraph of text to the commit message describing why the feature is useful.

A few nits on the code, but otherwise looks good! :)

>                  # Sanity check; is /dev/urandom readable? If so, use it to
                 # pre-fill netperf's buffers
-                self.run_simple(['dd', 'if=/dev/urandom', 'of=/dev/null', 'bs=1', 'count=1'], errmsg="Err")
-                netperf['buffer'] = '-F /dev/urandom'
+                fillFile = args['test_payload']
+                self.run_simple(['dd', f'if={fillFile}', 'of=/dev/null', 'bs=1', 'count=1'], errmsg="Err")
+                netperf['buffer'] = f'-F {fillFile}'

Flent runs on Python versions as old as 3.5, so we can't use f-strings, sadly...

> @@ -1022,10 +1023,14 @@ def check(self):
                 netperf['-e'] = True
 
             try:
+                # If --test-payload option is specified, use data from that file
+                # else use the default value /dev/urandom. 
+                # Below sanity check will be performed in either case.

I'm not sure if the sanity check makes sense for a user-specified file: it means we'll just silently ignore the option if the file is not readable. I think it's better to fail noisily. We could do this either by doing the check but erroring out if it fails (it's the default), or we could skip the test for custom values and just let netperf fail when it can't open it.

> @@ -378,6 +378,12 @@ def __call__(self, parser, namespace, values, option_string=None):
     "to decrease the socket buffer size. Can be specified multiple times, "
     "with each value corresponding to a stream of a test.")
 
+test_group.add_argument(
+    "--test-payload",
+    action="store", type=unicode, dest="TEST_PAYLOAD", default='/dev/urandom',
+    help="Path to file containing payload to pre-fill the netperf buffers with"

Missing space at the end of the first string here

>                  # Sanity check; is /dev/urandom readable? If so, use it to
                 # pre-fill netperf's buffers
-                self.run_simple(['dd', 'if=/dev/urandom', 'of=/dev/null', 'bs=1', 'count=1'], errmsg="Err")
-                netperf['buffer'] = '-F /dev/urandom'
+                fillFile = args['test_payload']

Variables are generally lowercase and underscore-separated rather then camelCase...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tohojo/flent/pull/243#pullrequestreview-783839744

[-- Attachment #2: Type: text/html, Size: 4775 bytes --]