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

improve redirect role input validation. Fixes #1651 #1652

Conversation

phillxnet
Copy link
Member

@phillxnet phillxnet commented Feb 14, 2017

Prior to this pr it was found that a non btrfs redirect role could be set on a device that also contained a btrfs partition. This was not intended behaviour and results in an import option that is non functional. This pr adds additional redirect role input validation to reject non btrfs partition redirection when a device also contains a btrfs partition. The check is only made when a role change is requested. This allows for the default whole disk wipe option.

As this is a bug fix for (my) code contributed in pr #1622 I have provided the additional manual testing steps required to identify and confirm the fix for this issue. These steps are additional to those detailed in #1622 .

Initial test drive partition setup(using sata bus this time):

yum install dosfstools
# 4GB virtual sata device (50% partitions give bare min of 2GB each) serial number "serial3"
parted -a optimal /dev/disk/by-id/ata-QEMU_HARDDISK_serial3
mklabel msdos
mkpart primary fat32 1 50%
mkpart primary ext2 50% 100%
quit
mkfs.fat -s2 -F 32 /dev/disk/by-id/ata-QEMU_HARDDISK_serial3-part1
mkfs.btrfs -f -L btrfs-in-partition /dev/disk/by-id/ata-QEMU_HARDDISK_serial3-part2
# the following command is required to ensure udev ‘sees’ the partition and fs changes.
udevadm trigger

Pre pr test failure:

status - no active redirect (default)

  • attempt to redirect to vfat partition - Allowed
    This is a FAIL as it is not intended behaviour.

Post pr test procedure:

status - no active redirect (default)

  • attempt to redirect to vfat partition - Blocked
  • resubmit whole disk selection - OK (no change)
  • wipe whole disk - OK

repeat above “Initial test drive partition setup” as disk is now blank.

status - no active redirect (default)

  • attempt to redirect to btrfs partition - OK

leading to the next series of tests:

status - active btrfs redirect:

  • attempt to redirect to vfat partition - Blocked
  • attempt to remove redirect (whole disk) - Blocked
  • wipe active btrfs partition - OK

leading to the next series of tests:

status - active non btrfs redirect (wiped in previous step):

  • attempt to redirect to vfat partition - OK
  • wipe active (vfat) redirect partition - OK
    (disk rescan required to update fs info)
  • attempt to remove redirect (whole disk) - OK
  • wipe whole disk - OK

During all of the "Post pr test procedure" steps above, the disk page 'flag' icons and their tool-tips, as well as any role config page warning messages, were as intended. Given the know issue identified in #1623 .

All manual tests detailed in pr #1622 were also re run and no regression was observed.

Fixes #1651

Fix inconsistency between validation and user messaging.
Add additional input validation more in line with original
user messaging: ie only allow the selection of a btrfs
partition if one exists. And only in cases where no btrfs
partition is found do we allow any partition to be selected.
@phillxnet
Copy link
Member Author

@schakrava Ready for review.

Requesting this pr be included in the current testing channel updates and prior to the next stable release.

Apologies to my fellow developers for missing this the first time around.

@schakrava
Copy link
Member

Thank you @phillxnet !

@schakrava schakrava merged commit 8ed4ea0 into rockstor:master Feb 19, 2017
@phillxnet
Copy link
Member Author

@schakrava Cheers.

@phillxnet phillxnet deleted the 1651_inconsistent_redirect_role_validator_message_and_bug branch June 6, 2017 17:56
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.

inconsistent redirect role validator message and bug
2 participants