mirror: Handle global parameters properly on mirror set

We have 2 global parameters to change content and version url that
were confusing when using mirror. For example user's didn't know what
to expect from:

$ swupd mirror --content-url https://new_url

or

$ swupd mirror --set https://new_url1 --content-url https://new_url2

Fix #941

Signed-off-by: Otavio Pontes <otavio.pontes@intel.com>

src/mirror.c

@@ -30,8 +30,9 @@
 
 #include "swupd.h"
 
-static char *set = NULL;
+static bool set = false;
 static bool unset = false;
+const char *set_url = NULL;
 
 static void print_help(void)
 {
@@ -43,17 +44,17 @@ static void print_help(void)
 	global_print_help();
 
 	print("Options:\n");
-	print("   -s, --set               set mirror url\n");
+	print("   -s, --set [URL]         set mirror url\n");
 	print("   -U, --unset             unset mirror url\n");
 	print("\n");
 }
 
 static const struct option prog_opts[] = {
-	{ "set", required_argument, 0, 's' },
+	{ "set", no_argument, 0, 's' },
 	{ "unset", no_argument, 0, 'U' },
 };
 
-static bool parse_opt(int opt, char *optarg)
+static bool parse_opt(int opt, UNUSED_PARAM char *optarg)
 {
 	switch (opt) {
 	case 's':
@@ -61,10 +62,10 @@ static bool parse_opt(int opt, char *optarg)
 			error("cannot set and unset at the same time\n");
 			return false;
 		}
-		set = optarg;
+		set = true;
 		return true;
 	case 'U':
-		if (set != NULL) {
+		if (set) {
 			error("cannot set and unset at the same time\n");
 			return false;
 		}
@@ -91,30 +92,38 @@ static bool parse_options(int argc, char **argv)
 		return false;
 	}
 
-	if (argc > ind) {
-		error("unexpected arguments\n\n");
+	if (!set && (globals.content_url || globals.version_url)) {
+		error("Version or content url can only be used with --set\n");
 		return false;
 	}
 
-	return true;
+	if (argc <= ind) {
+		// Check if URLs are set
+		if (set && !globals.content_url && !globals.version_url) {
+			error("Option '--set' requires an argument\n");
+			return false;
+		}
+		return true;
+	} else if (set && argc == ind + 1) {
+		set_url = argv[ind];
+		return true;
+	}
+
+	error("Unexpected arguments\n\n");
+	return false;
 }
 
 static int unset_mirror_url()
 {
 	char *content_path;
 	char *version_path;
-	int ret = 0;
+	int ret1 = 0, ret2 = 0;
 	content_path = mk_full_filename(globals.path_prefix, MIRROR_CONTENT_URL_PATH);
 	version_path = mk_full_filename(globals.path_prefix, MIRROR_VERSION_URL_PATH);
 
-	if ((ret = swupd_rm(content_path))) {
-		goto out;
-	}
-	if ((ret = swupd_rm(version_path))) {
-		goto out;
-	}
+	ret1 = swupd_rm(content_path);
+	ret2 = swupd_rm(version_path);
 
-out:
 	/* we need to also unset the mirror urls from the cache and set it to
 	 * the central version */
 	free_string(&globals.version_url);
@@ -123,10 +132,20 @@ out:
 	set_default_content_url();
 	free_string(&content_path);
 	free_string(&version_path);
-	return ret;
+
+	// No errors
+	if ((ret1 == -ENOENT && !ret2) ||
+	    (ret2 == -ENOENT && !ret1)) {
+		return 0;
+	}
+
+	if (ret1) {
+		return ret1;
+	}
+	return ret2;
 }
 
-static enum swupd_code write_to_path(char *content, char *path)
+static enum swupd_code write_to_path(const char *content, const char *path)
 {
 	char *dir, *tmp = NULL;
 	struct stat dirstat;
@@ -178,11 +197,10 @@ out:
 	return ret;
 }
 
-static enum swupd_code set_mirror_url(char *url)
+static enum swupd_code set_new_url(const char *url, const char *url_path)
 {
-	char *content_path;
-	char *version_path;
-	int ret = SWUPD_OK;
+	int ret;
+	char *path;
 
 	/* enforce the use of https */
 	if (!is_url_allowed(url)) {
@@ -191,34 +209,48 @@ static enum swupd_code set_mirror_url(char *url)
 
 	/* concatenate path_prefix and configuration paths if necessary
 	 * if path_prefix is NULL the second argument will be returned */
-	content_path = mk_full_filename(globals.path_prefix, MIRROR_CONTENT_URL_PATH);
-	version_path = mk_full_filename(globals.path_prefix, MIRROR_VERSION_URL_PATH);
-
-	/* write url to path_prefix/MIRROR_CONTENT_URL_PATH */
-	ret = write_to_path(url, content_path);
-	if (ret != SWUPD_OK) {
-		goto out;
-	}
+	path = mk_full_filename(globals.path_prefix, url_path);
 
 	/* write url to path_prefix/MIRROR_VERSION_URL_PATH */
-	ret = write_to_path(url, version_path);
-	if (ret != SWUPD_OK) {
-		goto out;
+	ret = write_to_path(url, path);
+	free(path);
+
+	return ret;
+}
+
+static enum swupd_code set_mirror_url(const char *url_new)
+{
+	int ret;
+	const char *content_url_new = globals.content_url;
+	const char *version_url_new = globals.version_url;
+
+	if (url_new) {
+		content_url_new = url_new;
+		version_url_new = url_new;
 	}
 
-	/* if mirror is http, warn user about needing to set
-	 * allow_insecure_http=true in order for auto-update to continue working */
-	if (is_url_insecure(url)) {
+	if (version_url_new) {
+		ret = set_new_url(version_url_new, MIRROR_VERSION_URL_PATH);
+		if (ret != SWUPD_OK) {
+			return ret;
+		}
+	}
+
+	if (content_url_new) {
+		ret = set_new_url(content_url_new, MIRROR_CONTENT_URL_PATH);
+		if (ret != SWUPD_OK) {
+			return ret;
+		}
+	}
+
+	if (is_url_insecure(content_url_new) || is_url_insecure(version_url_new)) {
 		warn("The mirror was set up using HTTP. In order for autoupdate "
 		     "to continue working you will need to set allow_insecure_http=true "
 		     "in the swupd configuration file. Alternatively you can set the "
 		     "mirror using HTTPS (recommended)\n\n");
 	}
 
-out:
-	free_string(&content_path);
-	free_string(&version_path);
-	return ret;
+	return SWUPD_OK;
 }
 
 static bool mirror_is_set(void)
@@ -321,13 +353,14 @@ enum swupd_code mirror_main(int argc, char **argv)
 	}
 	progress_init_steps("mirror", steps_in_mirror);
 
-	if (set != NULL) {
+	if (set) {
 		check_root();
-		ret = set_mirror_url(set);
+		ret = set_mirror_url(set_url);
 		if (ret != SWUPD_OK) {
-			warn("Unable to set mirror url\n");
+			error("Unable to set mirror url\n");
+			goto finish;
 		} else {
-			print("Set upstream mirror to %s\n", set);
+			print("Mirror url set\n");
 		}
 	} else if (unset) {
 		check_root();
@@ -337,7 +370,7 @@ enum swupd_code mirror_main(int argc, char **argv)
 			ret = SWUPD_OK;
 		} else if (ret != 0) {
 			ret = SWUPD_COULDNT_REMOVE_FILE;
-			warn("Unable to remove mirror configuration\n");
+			error("Unable to remove mirror configuration\n");
 		} else { /* ret == 0 */
 			print("Mirror url configuration removed\n");
 		}

test/functional/mirror/mirror-allow-http.bats

@@ -53,7 +53,7 @@ global_teardown() {
 		Warning: This is an insecure connection
 		The --allow-insecure-http flag was used, be aware that this poses a threat to the system
 		Warning: The mirror was set up using HTTP. In order for autoupdate to continue working you will need to set allow_insecure_http=true in the swupd configuration file. Alternatively you can set the mirror using HTTPS (recommended)
-		Set upstream mirror to http://example.com/swupd-file
+		Mirror url set
 		Distribution:      Swupd Test Distro
 		Installed version: 10
 		Version URL:       http://example.com/swupd-file

test/functional/mirror/mirror-createdir-negative.bats

@@ -37,11 +37,7 @@ global_teardown() {
 	assert_status_is_not 0
 	expected_output=$(cat <<-EOM
 		.*/etc/swupd: not a directory
-		Warning: Unable to set mirror url
-		Distribution:      Swupd Test Distro
-		Installed version: 10
-		Version URL:       file://.*/web-dir
-		Content URL:       file://.*/web-dir
+		Error: Unable to set mirror url
 	EOM
 	)
 	assert_regex_is_output "$expected_output"
@@ -59,11 +55,7 @@ global_teardown() {
 	assert_status_is_not 0
 	expected_output=$(cat <<-EOM
 		.*/etc/swupd: not a directory
-		Warning: Unable to set mirror url
-		Distribution:      Swupd Test Distro
-		Installed version: 10
-		Version URL:       file://.*/web-dir
-		Content URL:       file://.*/web-dir
+		Error: Unable to set mirror url
 	EOM
 	)
 	assert_regex_is_output "$expected_output"

test/functional/mirror/mirror-createdir.bats

@@ -33,7 +33,7 @@ global_teardown() {
 
 	assert_status_is 0
 	expected_output=$(cat <<-EOM
-		Set upstream mirror to https://example.com/swupd-file
+		Mirror url set
 		Distribution:      Swupd Test Distro
 		Installed version: 10
 		Version URL:       https://example.com/swupd-file
@@ -54,7 +54,7 @@ global_teardown() {
 
 	assert_status_is 0
 	expected_output=$(cat <<-EOM
-		Set upstream mirror to https://example.com/swupd-file
+		Mirror url set
 		Distribution:      Swupd Test Distro
 		Installed version: 10
 		Version URL:       https://example.com/swupd-file
@@ -76,7 +76,7 @@ global_teardown() {
 
 	assert_status_is 0
 	expected_output=$(cat <<-EOM
-		Set upstream mirror to https://example.com/swupd-file
+		Mirror url set
 		Distribution:      Swupd Test Distro
 		Installed version: 10
 		Version URL:       https://example.com/swupd-file

test/functional/mirror/mirror-json.bats

@@ -26,7 +26,7 @@ test_setup() {
 		{ "type" : "warning", "msg" : "This is an insecure connection " },
 		{ "type" : "info", "msg" : "The --allow-insecure-http flag was used, be aware that this poses a threat to the system  " },
 		{ "type" : "warning", "msg" : "The mirror was set up using HTTP. In order for autoupdate to continue working you will need to set allow_insecure_http=true in the swupd configuration file. Alternatively you can set the mirror using HTTPS (recommended)  " },
-		{ "type" : "info", "msg" : "Set upstream mirror to http://example.com/swupd-file " },
+		{ "type" : "info", "msg" : "Mirror url set " },
 		{ "type" : "info", "msg" : "Distribution:      Swupd Test Distro " },
 		{ "type" : "info", "msg" : "Installed version: 10 " },
 		{ "type" : "info", "msg" : "Version URL:       http://example.com/swupd-file " },

test/functional/mirror/mirror-set-unset-invalid.bats

@@ -0,0 +1,105 @@
+#!/usr/bin/env bats
+
+# Author: Otavio Pontes
+# Email: otavio.pontes@intel.com
+
+load "../testlib"
+
+global_setup() {
+
+	create_test_environment "$TEST_NAME"
+
+}
+
+test_setup() {
+
+	# do nothing
+	return
+
+}
+
+test_teardown() {
+
+	sudo rm -rf "$TARGETDIR"/etc/swupd
+
+}
+
+global_teardown() {
+
+	destroy_test_environment "$TEST_NAME"
+
+}
+
+@test "MIR016: Set invalid parameters" {
+
+	run sudo sh -c "$SWUPD mirror --set $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Option '--set' requires an argument
+	EOM
+	)
+	assert_in_output "$expected_output"
+
+	run sudo sh -c "$SWUPD mirror --set url1 url2 $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Unexpected arguments
+	EOM
+	)
+	assert_in_output "$expected_output"
+}
+
+@test "MIR017: Unset invalid parameters" {
+
+	run sudo sh -c "$SWUPD mirror --unset url $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Unexpected arguments
+	EOM
+	)
+	assert_in_output "$expected_output"
+
+	run sudo sh -c "$SWUPD mirror --unset -c url $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Version or content url can only be used with --set
+	EOM
+	)
+	assert_in_output "$expected_output"
+
+	run sudo sh -c "$SWUPD mirror --unset -v url $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Version or content url can only be used with --set
+	EOM
+	)
+	assert_in_output "$expected_output"
+
+	run sudo sh -c "$SWUPD mirror --unset -u url $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Version or content url can only be used with --set
+	EOM
+	)
+	assert_in_output "$expected_output"
+}
+
+@test "MIR018: Set/Unset invalid parameter" {
+
+	run sudo sh -c "$SWUPD mirror --set url --unset $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: cannot set and unset at the same time
+	EOM
+	)
+	assert_in_output "$expected_output"
+
+	run sudo sh -c "$SWUPD mirror --set url1 url2 $SWUPD_OPTS"
+	assert_status_is "$SWUPD_INVALID_OPTION"
+	expected_output=$(cat <<-EOM
+		Error: Unexpected arguments
+	EOM
+	)
+	assert_in_output "$expected_output"
+}
+

test/functional/mirror/mirror-set-unset.bats

@@ -0,0 +1,182 @@
+#!/usr/bin/env bats
+
+# Author: Otavio Pontes
+# Email: otavio.pontes@intel.com
+
+load "../testlib"
+
+global_setup() {
+
+	create_test_environment "$TEST_NAME"
+
+}
+
+test_setup() {
+
+	# do nothing
+	return
+
+}
+
+test_teardown() {
+
+	sudo rm -rf "$TARGETDIR"/etc/swupd
+
+}
+
+global_teardown() {
+
+	destroy_test_environment "$TEST_NAME"
+
+}
+
+@test "MIR011: Set/Unset a full mirror" {
+
+	run sudo sh -c "$SWUPD mirror --set https://example.com/swupd-file $SWUPD_OPTS"
+	assert_status_is 0
+	expected_output=$(cat <<-EOM
+		Mirror url set
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       https://example.com/swupd-file
+		Content URL:       https://example.com/swupd-file
+	EOM
+	)
+	assert_is_output "$expected_output"
+	assert_equal "https://example.com/swupd-file" "$(<"$TARGETDIR"/etc/swupd/mirror_contenturl)"
+	assert_equal "https://example.com/swupd-file" "$(<"$TARGETDIR"/etc/swupd/mirror_versionurl)"
+
+	run sudo sh -c "$SWUPD mirror --unset $SWUPD_OPTS"
+	assert_status_is 0
+
+	expected_output=$(cat <<-EOM
+		Mirror url configuration removed
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       file://$TEST_DIRNAME/web-dir
+		Content URL:       file://$TEST_DIRNAME/web-dir
+	EOM
+	)
+	assert_is_output "$expected_output"
+
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_contenturl
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_versionurl
+}
+
+@test "MIR012: Unsetting a mirror not set" {
+
+	run sudo sh -c "$SWUPD mirror --unset $SWUPD_OPTS"
+	assert_status_is 0
+
+	expected_output=$(cat <<-EOM
+		No mirror url configuration to remove
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       file://$TEST_DIRNAME/web-dir
+		Content URL:       file://$TEST_DIRNAME/web-dir
+	EOM
+	)
+	assert_is_output "$expected_output"
+
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_contenturl
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_versionurl
+}
+
+@test "MIR013: Set/Unset a full mirror with globals" {
+
+	run sudo sh -c "$SWUPD mirror --set -u https://example.com/swupd-file $SWUPD_OPTS"
+	assert_status_is 0
+	expected_output=$(cat <<-EOM
+		Mirror url set
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       https://example.com/swupd-file
+		Content URL:       https://example.com/swupd-file
+	EOM
+	)
+	assert_is_output "$expected_output"
+	assert_equal "https://example.com/swupd-file" "$(<"$TARGETDIR"/etc/swupd/mirror_contenturl)"
+	assert_equal "https://example.com/swupd-file" "$(<"$TARGETDIR"/etc/swupd/mirror_versionurl)"
+
+	run sudo sh -c "$SWUPD mirror --unset $SWUPD_OPTS"
+	assert_status_is 0
+
+	expected_output=$(cat <<-EOM
+		Mirror url configuration removed
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       file://$TEST_DIRNAME/web-dir
+		Content URL:       file://$TEST_DIRNAME/web-dir
+	EOM
+	)
+	assert_is_output "$expected_output"
+
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_contenturl
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_versionurl
+}
+
+@test "MIR014: Set/Unset a content mirror with globals" {
+
+	run sudo sh -c "$SWUPD mirror --set -c https://example.com/swupd-file $SWUPD_OPTS"
+	assert_status_is 0
+	expected_output=$(cat <<-EOM
+		Mirror url set
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       file://$TEST_DIRNAME/web-dir
+		Content URL:       https://example.com/swupd-file
+	EOM
+	)
+	assert_is_output "$expected_output"
+	assert_equal "https://example.com/swupd-file" "$(<"$TARGETDIR"/etc/swupd/mirror_contenturl)"
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_versionurl
+
+	run sudo sh -c "$SWUPD mirror --unset $SWUPD_OPTS"
+	assert_status_is 0
+
+	expected_output=$(cat <<-EOM
+		Mirror url configuration removed
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       file://$TEST_DIRNAME/web-dir
+		Content URL:       file://$TEST_DIRNAME/web-dir
+	EOM
+	)
+	assert_is_output "$expected_output"
+
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_contenturl
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_versionurl
+}
+
+@test "MIR015: Set/Unset a version mirror with globals" {
+
+	run sudo sh -c "$SWUPD mirror --set -v https://example.com/swupd-file $SWUPD_OPTS"
+	assert_status_is 0
+	expected_output=$(cat <<-EOM
+		Mirror url set
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       https://example.com/swupd-file
+		Content URL:       file://$TEST_DIRNAME/web-dir
+	EOM
+	)
+	assert_is_output "$expected_output"
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_contenturl
+	assert_equal "https://example.com/swupd-file" "$(<"$TARGETDIR"/etc/swupd/mirror_versionurl)"
+
+	run sudo sh -c "$SWUPD mirror --unset $SWUPD_OPTS"
+	assert_status_is 0
+
+	expected_output=$(cat <<-EOM
+		Mirror url configuration removed
+		Distribution:      Swupd Test Distro
+		Installed version: 10
+		Version URL:       file://$TEST_DIRNAME/web-dir
+		Content URL:       file://$TEST_DIRNAME/web-dir
+	EOM
+	)
+	assert_is_output "$expected_output"
+
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_contenturl
+	assert_file_not_exists "$TARGETDIR"/etc/swupd/mirror_versionurl
+}