cgdconfig verification method failure

[rudolf <netbsd%eq.cz@localhost>]

Hi,

(observed under netbsd-8, attached is an untested (but trivial) patch 
for current)

cgdconfig(8) currently (and under netbsd-8) behaves inconsistently 
regarding the verification method failure: if the verification method is 
"ffs" or "disklabel", then on a password mismatch it prompts for the 
passphrase again ("cgdconfig: verification failed, please reenter 
passphrase"). In case of verification methods "mbr" and "gpt", verify() 
returns -1 and thus cgdconfig(8) exits. This seems like a bug to me and 
should be fixed by the second and the third hunk of the diff to 
cgdconfig.c from the attached patch.

cgdconfig(8) also upon an exit due to the verification method failure 
leaves the cgd device configured (although correctly exiting with a 
non-zero exit status). This seems also like a bug to me (or an 
inconvenience) and should be fixed by the first hunk of the diff to 
cgdconfig.c from the attached patch. The amended tests from the patch 
cover this behavior (taking the fact that there is a configured device 
on verification method failure as a test failure). The fact that the 
devices are currently configured even after the verification failure 
seems to be in contradiction with the sentence from the cgdconfig(8) 
manual page (regarding the "-p" flag"): ``If this flag is specified then 
verification errors will cause the device in question to be unconfigured 
rather than prompting for the passphrase again.''

Kind regards,

r.

diff --git a/sbin/cgdconfig/cgdconfig.c b/sbin/cgdconfig/cgdconfig.c
index d1e035195866..c5a5738db4e9 100644
--- a/sbin/cgdconfig/cgdconfig.c
+++ b/sbin/cgdconfig/cgdconfig.c
@@ -626,6 +626,7 @@ configure(int argc, char **argv, struct params *inparams, int flags)
 
 		ret = verify(p, fd);
 		if (ret == -1)
+			(void)unconfigure_fd(fd);
 			goto bail_err;
 		if (!ret)
 			break;
@@ -830,7 +831,7 @@ verify_mbr(int fd)
 
 	memcpy(&mbr, buf, sizeof(mbr));
 	if (le16toh(mbr.mbr_magic) != MBR_MAGIC)
-		return -1;
+		return 1;
 
 	return 0;
 }
@@ -916,7 +917,7 @@ verify_gpt(int fd)
 		return -1;
 	}
 
-	ret = -1;
+	ret = 1;
 	for (blksize=DEV_BSIZE;
              (off = blksize * GPT_HDR_BLKNO) <= SCANSIZE - sizeof(hdr);
              blksize <<= 1) {
diff --git a/tests/dev/cgd/t_cgd.sh b/tests/dev/cgd/t_cgd.sh
index 9cd50a5fd86d..5320873b7162 100644
--- a/tests/dev/cgd/t_cgd.sh
+++ b/tests/dev/cgd/t_cgd.sh
@@ -150,10 +150,53 @@ unaligned_write_cleanup()
 	env RUMP_SERVER=unix://csock rump.halt || true
 }
 
+vmeth_failure_body()
+{
+
+	local vmeth="$1"
+	local d=$(atf_get_srcdir)
+
+	atf_check -s exit:0 \
+	    ${cgdserver} -d key=/dev/dk,hostpath=dk.img,size=1m unix://csock
+	export RUMP_SERVER=unix://csock
+	atf_check -s not-exit:0 -x "echo 12345 | \
+	    rump.cgdconfig -V "${vmeth}" -p cgd0 /dev/dk ${d}/paramsfile"
+	atf_check -s exit:0 -o not-match:"(^| )cgd0( |$)" rump.sysctl -n hw.disknames
+}
+
+test_case_vmeth_failure()
+{
+
+	local vmeth="${1}"
+	local name="vmeth_failure_${vmeth}"
+
+	atf_test_case "${name}" cleanup
+	eval "${name}_head() { \
+		atf_set "descr" "Tests verification method \"${vmeth}\" failure" ; \
+		atf_set "require.progs" "rump_server" ; \
+	}"
+	eval "${name}_body() { \
+		vmeth_failure_body "${vmeth}" ; \
+	}"
+	eval "${name}_cleanup() { \
+		rump.cgdconfig -u cgd0 2>/dev/null ; \
+		env RUMP_SERVER=unix://csock rump.halt || true ; \
+	}"
+}
+
+test_case_vmeth_failure disklabel
+test_case_vmeth_failure ffs
+test_case_vmeth_failure gpt
+test_case_vmeth_failure mbr
+
 atf_init_test_cases()
 {
 
 	atf_add_test_case basic
 	atf_add_test_case wrongpass
 	atf_add_test_case unaligned_write
+	atf_add_test_case vmeth_failure_disklabel
+	atf_add_test_case vmeth_failure_ffs
+	atf_add_test_case vmeth_failure_gpt
+	atf_add_test_case vmeth_failure_mbr
 }