Re: cgdconfig verification method failure

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

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

Attached is a tested and corrected version of the patch (differences to 
previous: 1. missing brackets, 2. ignore stderr of rump.cgdconfig in the 
tests).

> 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.''

r.

diff --git a/sbin/cgdconfig/cgdconfig.c b/sbin/cgdconfig/cgdconfig.c
index d1e035195866..cdc815fa05fb 100644
--- a/sbin/cgdconfig/cgdconfig.c
+++ b/sbin/cgdconfig/cgdconfig.c
@@ -625,8 +625,10 @@ configure(int argc, char **argv, struct params *inparams, int flags)
 			goto bail_err;
 
 		ret = verify(p, fd);
-		if (ret == -1)
+		if (ret == -1) {
+			(void)unconfigure_fd(fd);
 			goto bail_err;
+		}
 		if (!ret)
 			break;
 
@@ -830,7 +832,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 +918,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..f5ac322b4612 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 -e ignore -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
 }