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

Redacted Send/Receive causes zdb to dump core #8948

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Jun 22, 2019

Motivation and Context

When used with verbosity >= 4 zdb fails an assertion in dump_bookmarks() because it expects snprintf() to retun 0 on success.

root@master:~# zdb -ddddddd testpool/fs
Dataset testpool/fs [ZPL], ID 74, cr_txg 6, 24K, 6 objects, rootbp DVA[0]=<0:d000:200> DVA[1]=<0:100d000:200> [L0 DMU objset] fletcher4 lz4 unencrypted LE contiguous unique double size=1000L/200P birth=6L/6P fill=6 cksum=c5e39e0f3:4800b5c0ac7:dab03d77ffcb:1cd8517b60f997

    Deadlist: 0 (0/0 comp)

    mintxg 0 -> obj 77: object 77, 0 blkptrs, 0

    mintxg 1 -> obj 77: object 77, 0 blkptrs, 0

snprintf(buf, sizeof (buf), "%s#%s", osname, attr.za_name) == 0 (0xe == 0)
ASSERT at zdb.c:1943:dump_bookmarks()Aborted (core dumped)

Description

This was the only occurrence of zdb using VERIFY0() on snprintf() results, so i just removed it.

EDIT: gcc 7.3.1 20180303 does not like this (http://build.zfsonlinux.org/builders/Amazon%202%20x86_64%20%28BUILD%29/builds/12443/steps/shell_3/logs/make), change to VERIFY3S() instead.

How Has This Been Tested?

Run patched zdb binary on local builder, successfully use zdb -dddd testpool.

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:

@loli10K loli10K added the Status: Code Review Needed Ready for review and testing label Jun 22, 2019
When used with verbosity >= 4 zdb fails an assertion in dump_bookmarks()
because it expects snprintf() to retun 0 on success.

Signed-off-by: loli10K <[email protected]>
@loli10K loli10K force-pushed the fix_zdb_dump_bookmarks branch from 6814fbc to af58f21 Compare June 22, 2019 09:32
@codecov
Copy link

codecov bot commented Jun 22, 2019

Codecov Report

Merging #8948 into master will increase coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8948      +/-   ##
==========================================
+ Coverage   78.48%   78.52%   +0.03%     
==========================================
  Files         388      388              
  Lines      120013   120013              
==========================================
+ Hits        94197    94239      +42     
+ Misses      25816    25774      -42
Flag Coverage Δ
#kernel 79.44% <ø> (-0.02%) ⬇️
#user 65.87% <0%> (+0.01%) ⬆️

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 a370182...af58f21. Read the comment docs.

@ahrens ahrens requested a review from pcd1193182 June 24, 2019 17:21
Copy link
Contributor

@pcd1193182 pcd1193182 left a comment

Choose a reason for hiding this comment

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

Interesting. Not sure how this worked on Illumos, which appears to have the same return semantics, but this is definitely a good change.

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.

Do I understand correctly that the change to VERIFY3S resolved the gcc warning?

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 25, 2019
@behlendorf behlendorf merged commit 5279ae9 into openzfs:master Jun 25, 2019
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