Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZTS: Clean up zfs_clone_010_pos #9284

Merged
merged 1 commit into from
Sep 5, 2019

Conversation

ghost
Copy link

@ghost ghost commented Sep 4, 2019

Motivation and Context

On many systems, python is Python 3. zfs_clone_010_pos invokes python -c 'print "x" * 200', which is not valid in Python 3.

Description

Eliminate the Python, using built in shell constructs in its place.

While here, a few more potential improvements revealed themselves:

Remove a lot of unnecessary setting and incrementing of i.

Remove unused variable j.

Instead of generating the same string repeatedly inside a loop, generate it once outside of the loop.

How Has This Been Tested?

./scripts/zfs-tests.sh -vxT zfs_clone

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ghost ghost added the Status: Code Review Needed Ready for review and testing label Sep 4, 2019
@ghost ghost changed the title Clean up zfs_clone_010_pos ZTS: Clean up zfs_clone_010_pos Sep 4, 2019
@behlendorf behlendorf requested a review from jwk404 September 4, 2019 20:31
@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #9284 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9284      +/-   ##
==========================================
+ Coverage   79.06%   79.13%   +0.06%     
==========================================
  Files         400      400              
  Lines      122041   122041              
==========================================
+ Hits        96494    96578      +84     
+ Misses      25547    25463      -84
Flag Coverage Δ
#kernel 79.73% <ø> (-0.03%) ⬇️
#user 66.72% <ø> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d02186e...1e90971. Read the comment docs.

Remove a lot of unnecessary setting and incrementing of `i`.

Remove unused variable `j`.

Instead of calling out to Python in a loop to generate the same string
repeatedly, call it once outside of the loop. And add parenthesis to
make the print statement compatible with Python 3.

Signed-off-by: Ryan Moeller <ryan@ixsystems.com>
@ghost ghost force-pushed the zfs_clone_010-cleanup branch from 378236c to 1e90971 Compare September 5, 2019 13:15
@ghost
Copy link
Author

ghost commented Sep 5, 2019

I would like to amend the commit message to reflect the removal of Python after the tests have run. Should I use Requires-builders: style to avoid repeating the tests?

@behlendorf
Copy link
Contributor

@freqlabs it's OK to let the tests run again. The additional testing is always welcome. However, if you prefer you could simply post the updated commit message as a commit and I can fix up the comment when merging this.

@ghost
Copy link
Author

ghost commented Sep 5, 2019

@behlendorf thanks, to give the buildbots a chance to test someone else's work here is my updated commit message:

Clean up zfs_clone_010_pos

Remove a lot of unnecessary setting and incrementing of `i`.

Remove unused variable `j`.

Instead of calling out to Python in a loop to generate the same string
repeatedly, generate the string once using shell constructs before
entering the loop.

Signed-off-by: Ryan Moeller <ryan@ixsystems.com>

Copy link
Contributor

@ikozhukhov ikozhukhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for update

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 5, 2019
@behlendorf behlendorf merged commit 8e2c502 into openzfs:master Sep 5, 2019
@ghost ghost deleted the zfs_clone_010-cleanup branch September 6, 2019 00:51
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 24, 2019
Remove a lot of unnecessary setting and incrementing of `i`.

Remove unused variable `j`.

Instead of calling out to Python in a loop to generate the same string
repeatedly, generate the string once using shell constructs before
entering the loop.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Signed-off-by: Ryan Moeller <ryan@ixsystems.com>
Closes openzfs#9284
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
Remove a lot of unnecessary setting and incrementing of `i`.

Remove unused variable `j`.

Instead of calling out to Python in a loop to generate the same string
repeatedly, generate the string once using shell constructs before
entering the loop.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Signed-off-by: Ryan Moeller <ryan@ixsystems.com>
Closes openzfs#9284
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
Remove a lot of unnecessary setting and incrementing of `i`.

Remove unused variable `j`.

Instead of calling out to Python in a loop to generate the same string
repeatedly, generate the string once using shell constructs before
entering the loop.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Signed-off-by: Ryan Moeller <ryan@ixsystems.com>
Closes #9284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants