From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.github.com (out-18.smtp.github.com [192.30.252.201]) by mail.toke.dk (Postfix) with ESMTPS id 4D2E78D3872 for ; Wed, 20 Oct 2021 00:30:07 +0200 (CEST) Authentication-Results: mail.toke.dk; dkim=pass (1024-bit key) header.d=github.com header.i=@github.com header.b=wAXQDvPd Received: from github-lowworker-f144ac1.va3-iad.github.net (github-lowworker-f144ac1.va3-iad.github.net [10.48.16.59]) by smtp.github.com (Postfix) with ESMTP id D85E03408D0 for ; Tue, 19 Oct 2021 15:30:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1634682605; bh=aAa1Wwgt3F50XsTzf0y5MwvdXu1sBS4Xjg4uFk0DUpE=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=wAXQDvPd2W0xw27KbnOTrRTpB1W1p4NQq1cgyU19nmwoPdg4eUSjEGwxxB7lHzi6b tji962d/WPYd5rwpGaZpUN04umMRV8Pt98+BtyJxf/3ccF++qnnhMvSjtXvIJ5u4vU r8GVOg4VuNWJ1X6Ty9gqZixaVxeiXpssOvdUijuY= Date: Tue, 19 Oct 2021 15:30:05 -0700 From: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= To: tohojo/flent Message-ID: In-Reply-To: References: Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="--==_mimepart_616f46edc9fdf_64d6c710710fc"; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: list X-GitHub-Sender: tohojo X-GitHub-Recipient: flent-users X-GitHub-Reason: subscribed X-Auto-Response-Suppress: All X-GitHub-Recipient-Address: flent-users@flent.org Message-ID-Hash: G25GRCMBBCUBWE42XDH5QDGVD7K3CNHF X-Message-ID-Hash: G25GRCMBBCUBWE42XDH5QDGVD7K3CNHF X-MailFrom: noreply@github.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Subscribed X-Mailman-Version: 3.3.4 Reply-To: tohojo/flent Subject: [Flent-users] Re: [tohojo/flent] Added --test-payload option (PR #243) List-Id: Flent discussion list Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: ----==_mimepart_616f46edc9fdf_64d6c710710fc Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit @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 ----==_mimepart_616f46edc9fdf_64d6c710710fc Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

@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! :)

In flent/runners.py:

>                  # 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...

In flent/runners.py:

> @@ -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.

In flent/settings.py:

> @@ -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

In flent/runners.py:

>                  # 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, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

----==_mimepart_616f46edc9fdf_64d6c710710fc--