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

vdev_id: Return an error if config file is not found #12486

Closed
wants to merge 4 commits into from

Conversation

rugubara
Copy link
Contributor

Motivation and Context

The udev helper is supposed to return 0 exit code only when it produced the stdout suitable for consumption by the udev rule. error messages and usage messages should produce non-zero exit code that guarantees udev rule ignores the stdout.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Signed-off-by: Anton Gubarkov anton.gubarkov@gmail.com

@tonyhutter
Copy link
Contributor

I would change your commit message to:

vdev_id: Return an error if config file is not found

@tonyhutter
Copy link
Contributor

You'll also need a Signed-off-by: line in your commit message with your name and email. For example, mine is:

Signed-off-by: Tony Hutter <hutter2@llnl.gov>

Signed-off-by: Anton Gubarkov <anton.gubarkov@gmail.com>
@rugubara rugubara changed the title vdev_id udev helper returns 0 when stdout contains error/usage messages vdev_id: Return an error if config file is not found Aug 20, 2021
@rugubara
Copy link
Contributor Author

rugubara commented Aug 20, 2021

You'll also need a Signed-off-by: line in your commit message with your name and email. For example, mine is:

I pushed another commit with the signed-off line. I'm not sure if it is updated everywhere...

@tonyhutter
Copy link
Contributor

I think everything looks good. Could you squash your two commits into one commit? (git rebase -i master) I'll approve it after that.

@rugubara
Copy link
Contributor Author

I think everything looks good. Could you squash your two commits into one commit? (git rebase -i master) I'll approve it after that.

I tried... not sure that it gave the expected result.
My fork now has in my master branch:

commit 0e8cfee5a107a280cb7f4367c4c0df28d7cc0bbd
Author: Anton Gubarkov <anton.gubarkov@gmail.com>
Date:   Wed Aug 18 12:56:12 2021 +0300

    vdev_id: Return an error if config file is not found

    Signed-off-by: Anton Gubarkov <anton.gubarkov@gmail.com>

diff --git a/cmd/vdev_id/vdev_id b/cmd/vdev_id/vdev_id
index d349ba43c..cad59c93f 100755
--- a/cmd/vdev_id/vdev_id
+++ b/cmd/vdev_id/vdev_id
@@ -140,7 +140,8 @@ Usage: vdev_id [-h]
   -p    number of phy's per switch port [default=$PHYS_PER_PORT]
   -h    show this summary
 EOF
-       exit 0
+       exit 1
+       # exit with error to avoid processing usage message by a udev rule
 }

 map_slot() {
@@ -728,7 +729,7 @@ done

 if [ ! -r "$CONFIG" ] ; then
        echo "Error: Config file \"$CONFIG\" not found"
-       exit 0
+       exit 1
 fi

 if [ -z "$DEV" ] && [ -z "$ENCLOSURE_MODE" ] ; then

I can't see this commit in this PR

@tonyhutter
Copy link
Contributor

Ah I see, you created this PR on your master branch, which explains why the git rebase didn't work. Typically you create a separate branch for a PR. Here are the steps to create a separate branch with your commit in it:

# Create a new "vdev_id_error_code" branch on top of openzfs master
git remote add upstream https://github.com/openzfs/zfs.git 
git fetch upstream
git checkout upstream/master
git checkout -b vdev_id_error_code

# Include your vdev_id commit in this branch
git cherry-pick 0e8cfee5a

# Push your new branch out to github
git push -u origin vdev_id_error_code

After you run though these steps, you should then open this PR, click "edit" at the top right, and then change your branch from master to vdev_id_error_code.

@rugubara
Copy link
Contributor Author

Please pardon my ignorance in git. I really understand now how short sighted I was creating a PR from my master branch.
The commands you've provided really allowed me to move my commit to a new branch in my fork of zfs. However, I can't make changes in the way this PR is created. I need to change the from part from rugubara/zfs:master to rugubara/zfs:vdev_id_error_code. This part is not changeable. I can change only the base branch. I think it's easier to create a new PR then. Please advise.
Screenshot from 2021-08-23 22-31-25

@tonyhutter
Copy link
Contributor

tonyhutter commented Aug 23, 2021

@rugubara whoops, you're right, I thought you could change both the source and destination branch in github, but you can only change the destination branch. Just create a new PR with your new branch and I'll approve it.

@rugubara
Copy link
Contributor Author

rugubara commented Aug 24, 2021

I'm closing this PR. The new one is #12508. I wonder if it is possible to break a link between this PR and the master branch of my fork...

@rugubara rugubara closed this Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants