12721 would like svcadm disable -c

Reviewed by: Jason King <jason.brian.king@gmail.com>
Heckling from the gallery by: Joshua M. Clulow <josh@sysmgr.org>
Approved by: Dan McDonald <danmcd@joyent.com>

6 files changed, 281 insertions, 118 deletions

diff --git a/usr/src/cmd/svc/startd/libscf.c b/usr/src/cmd/svc/startd/libscf.c
index c4a3b20117..fe54b500b6 100644
--- a/usr/src/cmd/svc/startd/libscf.c
+++ b/usr/src/cmd/svc/startd/libscf.c
@@ -22,6 +22,7 @@
 /*
  * Copyright (c) 2004, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright 2012 Milan Jurik. All rights reserved.
+ * Copyright 2020 Joyent, Inc.
  */
 
 
@@ -1681,13 +1682,20 @@ libscf_set_deathrow(scf_instance_t *inst, int deathrow)
 }
 
 /*
+ * Since we're clearing the over-ridden enabled state for the service, we'll
+ * also take the opportunity to remove any comment.
+ *
  * Returns 0, ECONNABORTED, ECANCELED, or EPERM.
  */
 int
 libscf_delete_enable_ovr(scf_instance_t *inst)
 {
+	int r = scf_instance_delete_prop(inst, SCF_PG_GENERAL_OVR,
+	    SCF_PROPERTY_ENABLED);
+	if (r != 0)
+		return (r);
 	return (scf_instance_delete_prop(inst, SCF_PG_GENERAL_OVR,
-	    SCF_PROPERTY_ENABLED));
+	    SCF_PROPERTY_COMMENT));
 }
 
 /*
diff --git a/usr/src/cmd/svc/svcadm/svcadm.c b/usr/src/cmd/svc/svcadm/svcadm.c
index b51d218f45..c5a046565f 100644
--- a/usr/src/cmd/svc/svcadm/svcadm.c
+++ b/usr/src/cmd/svc/svcadm/svcadm.c
@@ -24,7 +24,7 @@
  */
 
 /*
- * Copyright 2015, Joyent, Inc. All rights reserved.
+ * Copyright 2020, Joyent, Inc. All rights reserved.
  */
 
 /*
@@ -82,6 +82,19 @@ struct ht_elt {
 };
 
 
+/*
+ * Callback data for enable/disable.
+ */
+#define	SET_ENABLED	0x1
+#define	SET_TEMPORARY	0x2
+#define	SET_RECURSIVE	0x4
+
+typedef struct {
+	char ed_comment[SCF_COMMENT_MAX_LENGTH];
+	int ed_flags;
+} enable_data_t;
+
+
 scf_handle_t *h;
 ssize_t max_scf_fmri_sz;
 static const char *emsg_permission_denied;
@@ -146,7 +159,7 @@ usage()
 	(void) fprintf(stderr, gettext(
 	"Usage: %1$s [-S <state>] [-v] [-Z | -z zone] [cmd [args ... ]]\n\n"
 	"\t%1$s enable [-rst] [<service> ...]\t- enable and online service(s)\n"
-	"\t%1$s disable [-st] [<service> ...]\t- disable and offline "
+	"\t%1$s disable [-c comment] [-st] [<service> ...] - disable "
 	"service(s)\n"
 	"\t%1$s restart [-d] [<service> ...]\t- restart specified service(s)\n"
 	"\t%1$s refresh [<service> ...]\t\t- re-read service configuration\n"
@@ -567,20 +580,150 @@ out:
 	return (ret);
 }
 
+static int
+delete_prop(const char *fmri, scf_instance_t *inst, const char *pgname,
+    const char *propname)
+{
+	int r = scf_instance_delete_prop(inst, pgname, propname);
+
+	switch (r) {
+	case 0:
+		break;
+
+	case ECANCELED:
+		uu_warn(emsg_no_service, fmri);
+		break;
+
+	case EACCES:
+		uu_warn(gettext("Could not delete %s/%s "
+		    "property of %s: backend access denied.\n"),
+		    pgname, propname, fmri);
+		break;
+
+	case EROFS:
+		uu_warn(gettext("Could not delete %s/%s "
+		    "property of %s: backend is read-only.\n"),
+		    pgname, propname, fmri);
+		break;
+
+	default:
+		bad_error("scf_instance_delete_prop", r);
+	}
+
+	return (r);
+}
+
 /*
- * Enable or disable inst, per enable.  If temp is true, set
- * general_ovr/enabled.  Otherwise set general/enabled and delete
- * general_ovr/enabled if it exists (order is important here: we don't want the
- * enabled status to glitch).
+ * Returns 0, EPERM, or EROFS.
+ */
+static int
+set_enabled_props(scf_propertygroup_t *pg, enable_data_t *ed)
+{
+	scf_transaction_entry_t *ent1;
+	scf_transaction_entry_t *ent2;
+	scf_transaction_t *tx;
+	scf_value_t *v2;
+	scf_value_t *v1;
+	int ret = 0, r;
+
+	if ((tx = scf_transaction_create(h)) == NULL ||
+	    (ent1 = scf_entry_create(h)) == NULL ||
+	    (ent2 = scf_entry_create(h)) == NULL ||
+	    (v1 = scf_value_create(h)) == NULL ||
+	    (v2 = scf_value_create(h)) == NULL)
+		scfdie();
+
+	for (;;) {
+		if (scf_transaction_start(tx, pg) == -1) {
+			switch (scf_error()) {
+			case SCF_ERROR_PERMISSION_DENIED:
+				ret = EPERM;
+				goto out;
+
+			case SCF_ERROR_BACKEND_READONLY:
+				ret = EROFS;
+				goto out;
+
+			default:
+				scfdie();
+			}
+		}
+
+		if (scf_transaction_property_change_type(tx, ent1,
+		    SCF_PROPERTY_ENABLED, SCF_TYPE_BOOLEAN) != 0) {
+			if (scf_error() != SCF_ERROR_NOT_FOUND)
+				scfdie();
+
+			if (scf_transaction_property_new(tx, ent1,
+			    SCF_PROPERTY_ENABLED, SCF_TYPE_BOOLEAN) != 0)
+				scfdie();
+		}
+
+		scf_value_set_boolean(v1, !!(ed->ed_flags & SET_ENABLED));
+
+		r = scf_entry_add_value(ent1, v1);
+		assert(r == 0);
+
+		if (scf_transaction_property_change_type(tx, ent2,
+		    SCF_PROPERTY_COMMENT, SCF_TYPE_ASTRING) != 0) {
+			if (scf_error() != SCF_ERROR_NOT_FOUND)
+				scfdie();
+
+			if (scf_transaction_property_new(tx, ent2,
+			    SCF_PROPERTY_COMMENT, SCF_TYPE_ASTRING) != 0)
+				scfdie();
+		}
+
+		if (scf_value_set_astring(v2, ed->ed_comment) != SCF_SUCCESS)
+			scfdie();
+
+		if (scf_entry_add_value(ent2, v2) != SCF_SUCCESS)
+			scfdie();
+
+		r = scf_transaction_commit(tx);
+		if (r == 1)
+			break;
+
+		scf_transaction_reset(tx);
+
+		if (r != 0) {
+			switch (scf_error()) {
+			case SCF_ERROR_PERMISSION_DENIED:
+				ret = EPERM;
+				goto out;
+
+			case SCF_ERROR_BACKEND_READONLY:
+				ret = EROFS;
+				goto out;
+
+			default:
+				scfdie();
+			}
+		}
+
+		if (scf_pg_update(pg) == -1)
+			scfdie();
+	}
+
+out:
+	scf_transaction_destroy(tx);
+	scf_entry_destroy(ent1);
+	scf_entry_destroy(ent2);
+	scf_value_destroy(v1);
+	scf_value_destroy(v2);
+	return (ret);
+}
+
+/*
+ * Enable or disable an instance.  SET_TEMPORARY modifications apply to
+ * general_ovr/ property group.
  */
 static void
-set_inst_enabled(const char *fmri, scf_instance_t *inst, boolean_t temp,
-    boolean_t enable)
+set_inst_enabled(const char *fmri, scf_instance_t *inst, enable_data_t *ed)
 {
 	scf_propertygroup_t *pg;
 	uint8_t b;
 	const char *pgname = NULL;	/* For emsg_pg_perm_denied */
-	int r;
 
 	pg = scf_pg_create(h);
 	if (pg == NULL)
@@ -625,14 +768,13 @@ set_inst_enabled(const char *fmri, scf_instance_t *inst, boolean_t temp,
 		}
 	}
 
-	if (temp) {
-		/* Set general_ovr/enabled */
+	if (ed->ed_flags & SET_TEMPORARY) {
 		pgname = SCF_PG_GENERAL_OVR;
 		if (pg_get_or_add(inst, pgname, SCF_PG_GENERAL_OVR_TYPE,
 		    SCF_PG_GENERAL_OVR_FLAGS, pg) != 0)
 			goto eperm;
 
-		switch (set_bool_prop(pg, SCF_PROPERTY_ENABLED, enable)) {
+		switch (set_enabled_props(pg, ed)) {
 		case 0:
 			break;
 
@@ -656,7 +798,7 @@ set_inst_enabled(const char *fmri, scf_instance_t *inst, boolean_t temp,
 		}
 
 		if (verbose)
-			(void) printf(enable ?
+			(void) printf((ed->ed_flags & SET_ENABLED) ?
 			    gettext("%s temporarily enabled.\n") :
 			    gettext("%s temporarily disabled.\n"), fmri);
 	} else {
@@ -671,7 +813,7 @@ again:
 		    SCF_PG_GENERAL_FLAGS, pg) != 0)
 			goto eperm;
 
-		switch (set_bool_prop(pg, SCF_PROPERTY_ENABLED, enable)) {
+		switch (set_enabled_props(pg, ed)) {
 		case 0:
 			break;
 
@@ -685,7 +827,7 @@ again:
 			 */
 			switch (get_bool_prop(pg, SCF_PROPERTY_ENABLED, &b)) {
 			case 0:
-				if ((b != 0) == (enable != B_FALSE))
+				if (!(b) == !(ed->ed_flags & SET_ENABLED))
 					break;
 				/* FALLTHROUGH */
 
@@ -716,39 +858,35 @@ again:
 			abort();
 		}
 
-		pgname = SCF_PG_GENERAL_OVR;
-		r = scf_instance_delete_prop(inst, pgname,
-		    SCF_PROPERTY_ENABLED);
-		switch (r) {
+		switch (delete_prop(fmri, inst, SCF_PG_GENERAL_OVR,
+		    SCF_PROPERTY_ENABLED)) {
 		case 0:
 			break;
 
-		case ECANCELED:
-			uu_warn(emsg_no_service, fmri);
-			goto out;
-
 		case EPERM:
 			goto eperm;
 
-		case EACCES:
-			uu_warn(gettext("Could not delete %s/%s "
-			    "property of %s: backend access denied.\n"),
-			    pgname, SCF_PROPERTY_ENABLED, fmri);
+		default:
 			goto out;
+		}
 
-		case EROFS:
-			uu_warn(gettext("Could not delete %s/%s "
-			    "property of %s: backend is read-only.\n"),
-			    pgname, SCF_PROPERTY_ENABLED, fmri);
-			goto out;
+		switch (delete_prop(fmri, inst, SCF_PG_GENERAL_OVR,
+		    SCF_PROPERTY_COMMENT)) {
+		case 0:
+			break;
+
+		case EPERM:
+			goto eperm;
 
 		default:
-			bad_error("scf_instance_delete_prop", r);
+			goto out;
 		}
 
-		if (verbose)
-			(void) printf(enable ?  gettext("%s enabled.\n") :
+		if (verbose) {
+			(void) printf((ed->ed_flags & SET_ENABLED) ?
+			    gettext("%s enabled.\n") :
 			    gettext("%s disabled.\n"), fmri);
+		}
 	}
 
 	scf_pg_destroy(pg);
@@ -1014,7 +1152,7 @@ out:
  * on cycle detection, or 0 on success.
  */
 static int
-enable_fmri_rec(char *fmri, boolean_t temp)
+enable_fmri_rec(char *fmri, enable_data_t *ed)
 {
 	scf_instance_t *inst;
 	scf_snapshot_t *snap;
@@ -1068,7 +1206,7 @@ enable_fmri_rec(char *fmri, boolean_t temp)
 		return (0);
 	}
 
-	set_inst_enabled(fmri, inst, temp, B_TRUE);
+	set_inst_enabled(fmri, inst, ed);
 
 	if ((snap = scf_snapshot_create(h)) == NULL ||
 	    (pg = scf_pg_create(h)) == NULL ||
@@ -1118,7 +1256,7 @@ enable_fmri_rec(char *fmri, boolean_t temp)
 		ret = 0;
 		goto out;
 	} else if (sz >= 0) {
-		switch (enable_fmri_rec(buf, temp)) {
+		switch (enable_fmri_rec(buf, ed)) {
 		case 0:
 			break;
 
@@ -1269,7 +1407,7 @@ enable_fmri_rec(char *fmri, boolean_t temp)
 			    -1)
 				scfdie();
 
-			switch (enable_fmri_rec(buf, temp)) {
+			switch (enable_fmri_rec(buf, ed)) {
 			case 0:
 				break;
 
@@ -1668,17 +1806,10 @@ out:
 }
 
 
-/*
- * Flags to control enable and disable actions.
- */
-#define	SET_ENABLED	0x1
-#define	SET_TEMPORARY	0x2
-#define	SET_RECURSIVE	0x4
-
 static int
 set_fmri_enabled(void *data, scf_walkinfo_t *wip)
 {
-	int flags = (int)data;
+	enable_data_t *ed = data;
 
 	assert(wip->inst != NULL);
 	assert(wip->pg == NULL);
@@ -1692,7 +1823,7 @@ set_fmri_enabled(void *data, scf_walkinfo_t *wip)
 			return (0);
 	}
 
-	if (flags & SET_RECURSIVE) {
+	if (ed->ed_flags & SET_RECURSIVE) {
 		char *fmri_buf = malloc(max_scf_fmri_sz);
 		if (fmri_buf == NULL)
 			uu_die(emsg_nomem);
@@ -1705,7 +1836,7 @@ set_fmri_enabled(void *data, scf_walkinfo_t *wip)
 		assert(strlen(wip->fmri) <= max_scf_fmri_sz);
 		(void) strlcpy(fmri_buf, wip->fmri, max_scf_fmri_sz);
 
-		switch (enable_fmri_rec(fmri_buf, (flags & SET_TEMPORARY))) {
+		switch (enable_fmri_rec(fmri_buf, ed)) {
 		case E2BIG:
 			uu_warn(gettext("operation on service %s is ambiguous; "
 			    "instance specification needed.\n"), fmri_buf);
@@ -1720,8 +1851,7 @@ set_fmri_enabled(void *data, scf_walkinfo_t *wip)
 		free(fmri_buf);
 
 	} else {
-		set_inst_enabled(wip->fmri, wip->inst,
-		    (flags & SET_TEMPORARY) != 0, (flags & SET_ENABLED) != 0);
+		set_inst_enabled(wip->fmri, wip->inst, ed);
 	}
 
 	return (0);
@@ -1972,7 +2102,6 @@ set_milestone(const char *fmri, boolean_t temporary)
 {
 	scf_instance_t *inst;
 	scf_propertygroup_t *pg;
-	int r;
 
 	if (temporary) {
 		set_astring_prop(SCF_SERVICE_STARTD, SCF_PG_OPTIONS_OVR,
@@ -1998,43 +2127,10 @@ set_milestone(const char *fmri, boolean_t temporary)
 	    SCF_PG_OPTIONS_TYPE, SCF_PG_OPTIONS_FLAGS, SCF_PROPERTY_MILESTONE,
 	    fmri);
 
-	r = scf_instance_delete_prop(inst, SCF_PG_OPTIONS_OVR,
-	    SCF_PROPERTY_MILESTONE);
-	switch (r) {
-	case 0:
-		break;
-
-	case ECANCELED:
-		uu_warn(emsg_no_service, fmri);
+	if (delete_prop(SCF_SERVICE_STARTD, inst, SCF_PG_OPTIONS_OVR,
+	    SCF_PROPERTY_MILESTONE) != 0)
 		exit_status = 1;
-		goto out;
 
-	case EPERM:
-		uu_warn(gettext("Could not delete %s/%s property of "
-		    "%s: permission denied.\n"), SCF_PG_OPTIONS_OVR,
-		    SCF_PROPERTY_MILESTONE, SCF_SERVICE_STARTD);
-		exit_status = 1;
-		goto out;
-
-	case EACCES:
-		uu_warn(gettext("Could not delete %s/%s property of "
-		    "%s: access denied.\n"), SCF_PG_OPTIONS_OVR,
-		    SCF_PROPERTY_MILESTONE, SCF_SERVICE_STARTD);
-		exit_status = 1;
-		goto out;
-
-	case EROFS:
-		uu_warn(gettext("Could not delete %s/%s property of "
-		    "%s: backend read-only.\n"), SCF_PG_OPTIONS_OVR,
-		    SCF_PROPERTY_MILESTONE, SCF_SERVICE_STARTD);
-		exit_status = 1;
-		goto out;
-
-	default:
-		bad_error("scf_instance_delete_prop", r);
-	}
-
-out:
 	scf_pg_destroy(pg);
 	scf_instance_destroy(inst);
 }
@@ -2315,7 +2411,10 @@ again:
 		usage();
 
 	if (strcmp(argv[optind], "enable") == 0) {
-		int flags = SET_ENABLED;
+		enable_data_t ed = {
+			.ed_flags = SET_ENABLED,
+			.ed_comment = ""
+		};
 		int wait = 0;
 		int error = 0;
 
@@ -2323,9 +2422,9 @@ again:
 
 		while ((o = getopt(argc, argv, "rst")) != -1) {
 			if (o == 'r')
-				flags |= SET_RECURSIVE;
+				ed.ed_flags |= SET_RECURSIVE;
 			else if (o == 't')
-				flags |= SET_TEMPORARY;
+				ed.ed_flags |= SET_TEMPORARY;
 			else if (o == 's')
 				wait = 1;
 			else if (o == '?')
@@ -2351,7 +2450,7 @@ again:
 		 * the errors the first time.
 		 */
 		if ((err = scf_walk_fmri(h, argc, argv, WALK_FLAGS,
-		    set_fmri_enabled, (void *)flags, &error, pr_warn)) != 0) {
+		    set_fmri_enabled, &ed, &error, pr_warn)) != 0) {
 
 			pr_warn(gettext("failed to iterate over "
 			    "instances: %s\n"), scf_strerror(err));
@@ -2359,7 +2458,7 @@ again:
 
 		} else if (wait && exit_status == 0 &&
 		    (err = scf_walk_fmri(h, argc, argv, WALK_FLAGS,
-		    wait_fmri_enabled, (void *)flags, &error, quiet)) != 0) {
+		    wait_fmri_enabled, NULL, &error, quiet)) != 0) {
 
 			pr_warn(gettext("failed to iterate over "
 			    "instances: %s\n"), scf_strerror(err));
@@ -2370,15 +2469,25 @@ again:
 			exit_status = error;
 
 	} else if (strcmp(argv[optind], "disable") == 0) {
-		int flags = 0;
+		enable_data_t ed = {
+			.ed_flags = 0,
+			.ed_comment = ""
+		};
 		int wait = 0;
 		int error = 0;
 
 		++optind;
 
-		while ((o = getopt(argc, argv, "st")) != -1) {
-			if (o == 't')
-				flags |= SET_TEMPORARY;
+		while ((o = getopt(argc, argv, "c:st")) != -1) {
+			if (o == 'c') {
+				if (strlcpy(ed.ed_comment, optarg,
+				    sizeof (ed.ed_comment)) >=
+				    sizeof (ed.ed_comment)) {
+					uu_die(gettext("disable -c comment "
+					    "too long.\n"));
+				}
+			} else if (o == 't')
+				ed.ed_flags |= SET_TEMPORARY;
 			else if (o == 's')
 				wait = 1;
 			else if (o == '?')
@@ -2404,7 +2513,7 @@ again:
 		 * the errors the first time.
 		 */
 		if ((err = scf_walk_fmri(h, argc, argv, WALK_FLAGS,
-		    set_fmri_enabled, (void *)flags, &exit_status,
+		    set_fmri_enabled, &ed, &exit_status,
 		    pr_warn)) != 0) {
 
 			pr_warn(gettext("failed to iterate over "
@@ -2413,7 +2522,7 @@ again:
 
 		} else if (wait && exit_status == 0 &&
 		    (err = scf_walk_fmri(h, argc, argv, WALK_FLAGS,
-		    wait_fmri_disabled, (void *)flags, &error, quiet)) != 0) {
+		    wait_fmri_disabled, NULL, &error, quiet)) != 0) {
 
 			pr_warn(gettext("failed to iterate over "
 			    "instances: %s\n"), scf_strerror(err));
diff --git a/usr/src/cmd/svc/svccfg/svccfg_libscf.c b/usr/src/cmd/svc/svccfg/svccfg_libscf.c
index 9fec777bd3..38dd45c75c 100644
--- a/usr/src/cmd/svc/svccfg/svccfg_libscf.c
+++ b/usr/src/cmd/svc/svccfg/svccfg_libscf.c
@@ -21,7 +21,7 @@
 
 /*
  * Copyright (c) 2004, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright 2019 Joyent, Inc.
+ * Copyright 2020 Joyent, Inc.
  * Copyright 2012 Milan Jurik. All rights reserved.
  * Copyright 2017 RackTop Systems.
  * Copyright 2018 OmniOS Community Edition (OmniOSce) Association.
@@ -10522,6 +10522,8 @@ export_inst_general(scf_propertygroup_t *pg, xmlNodePtr inode,
 
 		if (strcmp(exp_str, scf_property_enabled) == 0) {
 			continue;
+		} else if (strcmp(exp_str, SCF_PROPERTY_COMMENT) == 0) {
+			continue;
 		} else if (strcmp(exp_str, SCF_PROPERTY_RESTARTER) == 0) {
 			xmlNodePtr rnode, sfnode;
 
diff --git a/usr/src/cmd/svc/svcs/explain.c b/usr/src/cmd/svc/svcs/explain.c
index 533437cbee..331a5375fd 100644
--- a/usr/src/cmd/svc/svcs/explain.c
+++ b/usr/src/cmd/svc/svcs/explain.c
@@ -22,7 +22,7 @@
 /*
  * Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
- * Copyright (c) 2015, Joyent, Inc. All rights reserved.
+ * Copyright 2020 Joyent, Inc.
  */
 
 /*
@@ -133,6 +133,8 @@ typedef struct {
 	const char *restarter;
 	uu_list_t *dependencies;	/* list of dependency_group's */
 
+	char comment[SCF_COMMENT_MAX_LENGTH];
+
 	int active;			/* In use?  (cycle detection) */
 	int restarter_bad;
 	const char *summary;
@@ -403,7 +405,7 @@ add_instance(const char *svcname, const char *instname, scf_instance_t *inst)
 {
 	inst_t *instp;
 	svc_t *svcp;
-	int have_enabled = 0;
+	int ovr_set = 0;
 	uint8_t i;
 	uint32_t h;
 	int r;
@@ -479,7 +481,10 @@ add_instance(const char *svcname, const char *instname, scf_instance_t *inst)
 	if (scf_instance_get_pg(inst, SCF_PG_GENERAL_OVR, g_pg) == 0) {
 		if (pg_get_single_val(g_pg, SCF_PROPERTY_ENABLED,
 		    SCF_TYPE_BOOLEAN, &instp->enabled, 0, 0) == 0)
-			have_enabled = 1;
+			ovr_set = 1;
+		(void) pg_get_single_val(g_pg, SCF_PROPERTY_COMMENT,
+		    SCF_TYPE_ASTRING, instp->comment,
+		    sizeof (instp->comment), EMPTY_OK);
 	} else {
 		switch (scf_error()) {
 		case SCF_ERROR_NOT_FOUND:
@@ -508,11 +513,18 @@ add_instance(const char *svcname, const char *instname, scf_instance_t *inst)
 	if (pg_get_single_val(g_pg, SCF_PROPERTY_ENABLED, SCF_TYPE_BOOLEAN,
 	    &i, 0, 0) != 0)
 		return;
-	if (!have_enabled) {
+
+	if (ovr_set) {
+		instp->temporary = (instp->enabled != i);
+	} else {
 		instp->enabled = i;
 		instp->temporary = 0;
-	} else {
-		instp->temporary = (instp->enabled != i);
+	}
+
+	if (!instp->temporary) {
+		(void) pg_get_single_val(g_pg, SCF_PROPERTY_COMMENT,
+		    SCF_TYPE_ASTRING, instp->comment,
+		    sizeof (instp->comment), EMPTY_OK);
 	}
 
 	if (pg_get_single_val(g_pg, SCF_PROPERTY_RESTARTER, SCF_TYPE_ASTRING,
@@ -1736,12 +1748,23 @@ print_reasons(const inst_t *svcp, int verbose)
 
 	} else if (strcmp(svcp->state, SCF_STATE_STRING_DISABLED) == 0) {
 		if (!svcp->temporary) {
-			(void) puts(gettext(
-			    "Reason: Disabled by an administrator."));
+			if (svcp->comment[0] != '\0') {
+				(void) printf(gettext("Reason: Disabled by "
+				    "an administrator: %s\n"), svcp->comment);
+			} else {
+				(void) printf(gettext("Reason: Disabled by "
+				    "an administrator.\n"));
+			}
 			dc = DC_DISABLED;
 		} else {
-			(void) puts(gettext("Reason: "
-			    "Temporarily disabled by an administrator."));
+			if (svcp->comment[0] != '\0') {
+				(void) printf(gettext("Reason: Temporarily "
+				    "disabled by an administrator: %s\n"),
+				    svcp->comment);
+			} else {
+				(void) printf(gettext("Reason: Temporarily "
+				    "disabled by an administrator.\n"));
+			}
 			dc = DC_TEMPDISABLED;
 		}
 
diff --git a/usr/src/cmd/svc/svcs/svcs.c b/usr/src/cmd/svc/svcs/svcs.c
index ec3dba52d1..b6cf4f7fd1 100644
--- a/usr/src/cmd/svc/svcs/svcs.c
+++ b/usr/src/cmd/svc/svcs/svcs.c
@@ -21,7 +21,7 @@
 
 /*
  * Copyright (c) 2004, 2010, Oracle and/or its affiliates. All rights reserved.
- * Copyright 2019 Joyent, Inc.
+ * Copyright 2020 Joyent, Inc.
  * Copyright (c) 2015, 2016 by Delphix. All rights reserved.
  */
 
@@ -87,10 +87,6 @@
 
 #define	LEGACY_UNKNOWN	"unknown"
 
-/* Flags for pg_get_single_val() */
-#define	EMPTY_OK	0x01
-#define	MULTI_OK	0x02
-
 /*
  * Per proc(4) when pr_nlwp, pr_nzomb, and pr_lwp.pr_lwpid are all 0,
  * the process is a zombie.
@@ -409,7 +405,13 @@ pg_get_single_val(scf_propertygroup_t *pg, const char *propname, scf_type_t ty,
 
 	switch (ty) {
 	case SCF_TYPE_ASTRING:
-		r = scf_value_get_astring(g_val, vp, sz) > 0 ? SCF_SUCCESS : -1;
+		r = scf_value_get_astring(g_val, vp, sz);
+		if (r == 0 && !(flags & EMPTY_OK)) {
+			uu_die(gettext("Unexpected empty string for property "
+			    "%s.  Exiting.\n"), propname);
+		}
+		if (r >= 0)
+			r = SCF_SUCCESS;
 		break;
 
 	case SCF_TYPE_BOOLEAN:
@@ -2477,6 +2479,21 @@ print_detailed(void *unused, scf_walkinfo_t *wip)
 		    perm ? gettext("true") : gettext("false"));
 	}
 
+	if (temp == 0 || (temp == -1 && perm == 0)) {
+		char comment[SCF_COMMENT_MAX_LENGTH] = "";
+		const char *pg = (temp != -1 && temp != perm) ?
+		    SCF_PG_GENERAL_OVR : SCF_PG_GENERAL;
+
+		(void) inst_get_single_val(wip->inst, pg, SCF_PROPERTY_COMMENT,
+		    SCF_TYPE_ASTRING, &comment, sizeof (comment),
+		    EMPTY_OK, 0, 0);
+
+		if (comment[0] != '\0') {
+			printf(fmt, DETAILED_WIDTH, gettext("comment"),
+			    comment);
+		}
+	}
+
 	/*
 	 * Property values may be longer than max_scf_fmri_length, but these
 	 * shouldn't be, so we'll just reuse buf.  The user can use svcprop if
diff --git a/usr/src/cmd/svc/svcs/svcs.h b/usr/src/cmd/svc/svcs/svcs.h
index 5f0cb83360..78b3c26dd3 100644
--- a/usr/src/cmd/svc/svcs/svcs.h
+++ b/usr/src/cmd/svc/svcs/svcs.h
@@ -22,13 +22,13 @@
 /*
  * Copyright 2004 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
+ *
+ * Copyright 2020 Joyent, Inc.
  */
 
 #ifndef	_SVCS_H
 #define	_SVCS_H
 
-#pragma ident	"%Z%%M%	%I%	%E% SMI"
-
 #include <libscf.h>
 
 #ifdef	__cplusplus
@@ -52,6 +52,10 @@ void scfdie(void);
 void *safe_malloc(size_t);
 char *safe_strdup(const char *);
 
+/* Flags for pg_get_single_val() */
+#define	EMPTY_OK	0x01
+#define	MULTI_OK	0x02
+
 int pg_get_single_val(scf_propertygroup_t *, const char *, scf_type_t, void *,
     size_t, uint_t);
 int inst_get_single_val(scf_instance_t *, const char *, const char *,