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

Factor Linux specific functionality out of libzutil #9356

Merged
merged 1 commit into from
Oct 3, 2019

Conversation

mattmacy
Copy link
Contributor

@mattmacy mattmacy commented Sep 24, 2019

Signed-off-by: Matt Macy [email protected]

Motivation and Context

Description

How Has This Been Tested?

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 Sep 24, 2019
@ghost ghost added Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Code Review Needed Ready for review and testing labels Sep 25, 2019
@ghost
Copy link

ghost commented Sep 25, 2019

I think we can do a better job sharing code here, we're going to have another look.

@ghost ghost force-pushed the projects/libzutil_refactor branch from b0ed9c5 to 0798708 Compare September 26, 2019 23:02
@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Revision Needed Changes are required for the PR to be accepted labels Sep 27, 2019
@ghost ghost force-pushed the projects/libzutil_refactor branch 3 times, most recently from eebd10c to 96bbdc4 Compare September 27, 2019 20:01
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.

lib/libzutil/os/linux/zutil_import_os.c Outdated Show resolved Hide resolved
lib/libzutil/os/linux/zutil_import_os.c Outdated Show resolved Hide resolved
@@ -2045,21 +1252,19 @@ zpool_find_import_impl(libpc_handle_t *hdl, importargs_t *iarg)
pthread_mutex_init(&lock, NULL);

/*
* Locate pool member vdevs using libblkid or by directory scanning.
* Locate pool member vdevs by blkid or by directory scanning.
Copy link
Contributor

Choose a reason for hiding this comment

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

On FreeBSD is there an blkid equivalent which can be used here? Or will only the scan and cache options be implemented?

Copy link

Choose a reason for hiding this comment

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

There may be a possibility of using libblkid (I see a port for it anyway) but what I anticipate will end up here on FreeBSD is the code we have to scan our GEOM tree.

lib/libzutil/zutil_import.c Show resolved Hide resolved
@ghost ghost force-pushed the projects/libzutil_refactor branch from 96bbdc4 to ffd89cf Compare September 30, 2019 16:06
@ghost
Copy link

ghost commented Sep 30, 2019

  • Move Linux specific functions from zutil_device_path.c to os/linux/zutil_device_path_os.c.
  • Use boolean_t as return type for zfs_dev_is_* predicates and adjust the caller accordingly. This matches the comment at the top of one of the functions; the other function's comment was updated to match.
  • Share definition of struct libpc_handle with os specific implementation to remove #ifdef notyet.
  • Provide real definitions for some functions when building without libudev.
  • Make zutil_import.h include consistent.

@ghost ghost force-pushed the projects/libzutil_refactor branch from ffd89cf to 1ad1365 Compare September 30, 2019 16:21
@ghost
Copy link

ghost commented Sep 30, 2019

  • Add missing includes to os/linux/zutil_device_path_os.c.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 1, 2019
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.

Looks good. Let's just address the zfs_verror nit, and then we can plan to merge this tomorrow.

lib/libzutil/zutil_import.c Show resolved Hide resolved
@ghost ghost force-pushed the projects/libzutil_refactor branch from 1ad1365 to f9689b3 Compare October 2, 2019 13:55
@ghost
Copy link

ghost commented Oct 2, 2019

  • zfs_verror -> zutil_verror

@behlendorf
Copy link
Contributor

Thanks, looks good. Can you just rebase this to resole the conflict due to some other recent merges.

@ghost ghost force-pushed the projects/libzutil_refactor branch from f9689b3 to 989090f Compare October 2, 2019 18:03
Signed-off-by: Matt Macy <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
@ghost ghost force-pushed the projects/libzutil_refactor branch from 989090f to 831b0f9 Compare October 2, 2019 18:10
@ghost
Copy link

ghost commented Oct 2, 2019

^^ Rebase
^ Remove leftover devid.h include

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #9356 into master will increase coverage by 12.48%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9356       +/-   ##
===========================================
+ Coverage   66.62%    79.1%   +12.48%     
===========================================
  Files         326      406       +80     
  Lines      105097   122527    +17430     
===========================================
+ Hits        70017    96925    +26908     
+ Misses      35080    25602     -9478
Flag Coverage Δ
#kernel 79.79% <ø> (?)
#user 66.83% <75%> (+0.21%) ⬆️

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 8ae8b2a...831b0f9. Read the comment docs.

@behlendorf behlendorf merged commit 7c5eff9 into openzfs:master Oct 3, 2019
@mattmacy mattmacy deleted the projects/libzutil_refactor branch December 19, 2019 22:29
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.

2 participants