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

fix typesetting of Errata #4 #8721

Merged
merged 1 commit into from
May 8, 2019
Merged

fix typesetting of Errata #4 #8721

merged 1 commit into from
May 8, 2019

Conversation

JMoVS
Copy link
Contributor

@JMoVS JMoVS commented May 7, 2019

fixes #8712

Signed-off-by: Justin Scholz git@justinscholz.de

Added some missing tabs and newlines, was not able to verify myself though that it fixes the problem. Could someone with access to a linux verify before merging? For both the import and the status callback?

Motivation and Context

Description

Tried to catch all tabs and new lines that were still missing

How Has This Been Tested?

Wasn't able to test, please verify it before merging.

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:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 7, 2019
Signed-off-by: Justin Scholz <git@justinscholz.de>
Issue openzfs#8712
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

@JMoVS after locally testing the formatting of this PR I spotted a couple of lines which slightly exceeded the 80 character limit. While I had a test system set up I took the liberty of updating the formatting, the resulting output looks like this:

$ zpool import
   pool: tank
     id: 13002386651819330324
  state: ONLINE
 status: Errata #4 detected.
 action: Existing encrypted snapshots and bookmarks contain an on-disk
        incompatibility. This may cause on-disk corruption if they are used
        with 'zfs recv'. To correct the issue, enable the bookmark_v2 feature.
        No additional action is needed if there are no encrypted snapshots or
        bookmarks. If preserving the encrypted snapshots and bookmarks is
        required, use a non-raw send to backup and restore them. Alternately,
        they may be removed to resolve the incompatibility.
   see: http://zfsonlinux.org/msg/ZFS-8000-ER
 config:

        tank        ONLINE
          vdc       ONLINE
$ zpool import tank
$ zpool status
  pool: tank
 state: ONLINE
status: Errata #4 detected.
        Existing encrypted snapshots and bookmarks contain an on-disk
        incompatibility. This may cause on-disk corruption if they are used
        with 'zfs recv'.
action: To correct the issue, enable the bookmark_v2 feature. No additional
        action is needed if there are no encrypted snapshots or bookmarks.
        If preserving the encrypted snapshots and bookmarks is required, use
        a non-raw send to backup and restore them. Alternately, they may be
        removed to resolve the incompatibility.
   see: http://zfsonlinux.org/msg/ZFS-8000-ER
  scan: none requested
config:

        NAME        STATE     READ WRITE CKSUM
        tank        ONLINE       0     0     0
          vdc       ONLINE       0     0     0

errors: No known data errors

Would you mind updating this PR using behlendorf/zfs@b509b15. Or, if you prefer, I can open a replacement PR. Thanks.

@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Code Review Needed Ready for review and testing labels May 7, 2019
@JMoVS
Copy link
Contributor Author

JMoVS commented May 7, 2019

@behlendorf I can update, but in your output in the first case, there are 7 spaces in the 7 lines after the first missing at the beginning including in the "see" line.

@JMoVS
Copy link
Contributor Author

JMoVS commented May 7, 2019

@behlendorf Thanks for testing and the fix! I pulled your branch and force-pushed, so this PR should be updated properly now.

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

Merging #8721 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #8721     +/-   ##
=========================================
+ Coverage   78.73%   78.83%   +0.1%     
=========================================
  Files         381      381             
  Lines      117674   117630     -44     
=========================================
+ Hits        92649    92734     +85     
+ Misses      25025    24896    -129
Flag Coverage Δ
#kernel 79.26% <ø> (-0.05%) ⬇️
#user 67.75% <100%> (+0.51%) ⬆️

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 1f02ecc...b509b15. Read the comment docs.

@rlaager
Copy link
Member

rlaager commented May 8, 2019

@JMoVS I'm confused about the status of this. To me, the first output from @behlendorf does not line up.

@JMoVS
Copy link
Contributor Author

JMoVS commented May 8, 2019

@rlaager I've briefly chatted with @behlendorf about it today in the morning, here's from IRC:

00:36 <+behlendorf> JMoVS I'm pretty sure that must be a cut and paste error since the patch uses tabs there. Sorry about that.

@behlendorf behlendorf requested a review from ofaaland May 8, 2019 18:04
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 8, 2019
@behlendorf
Copy link
Contributor

To me, the first output from @behlendorf does not line up.

The alignment of the zpool import output is unfortunately expected. The left 'column' of this output has been one character wider than the zpool status output (9 vs 8) for a while now, and all of the 'action' messages are indented with a single tab.

Offhand I'm not sure why this is the case, and I agree making the alignment consistent is the right thing to do. But I think addressing that is outside the scope of this PR, and it really warrants its own.

@rlaager assuming you agree could you review and approve this PR.

Copy link
Contributor

@ofaaland ofaaland left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

This LGTM, subject to the caveat that I didn't run this and only reviewed the output as pasted here.

@behlendorf behlendorf merged commit e2dddb7 into openzfs:master May 8, 2019
allanjude pushed a commit to allanjude/zfs that referenced this pull request Jun 7, 2019
Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Justin Scholz <git@justinscholz.de>
Closes openzfs#8712 
Closes openzfs#8721
allanjude pushed a commit to allanjude/zfs that referenced this pull request Jun 15, 2019
Reviewed-by: Olaf Faaland <faaland1@llnl.gov>
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Justin Scholz <git@justinscholz.de>
Closes openzfs#8712 
Closes openzfs#8721
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.

Errata #4 warning has a very long line due to a missing newline
4 participants