18630 cannot publish an action where the payload hash contains an equals
authorDanek Duvall <danek.duvall@oracle.com>
Mon, 11 Jul 2011 16:34:01 -0700
changeset 2457 75e0d8b7d44f
parent 2456 063737b8ee9b
child 2458 7c1227ad555e
18630 cannot publish an action where the payload hash contains an equals 18648 underscore removal, part 1: informational attributes
doc/guide-metadata-conventions.rst
doc/tags-and-attributes.txt
src/client.py
src/man/pkg.5.txt
src/modules/actions/_actions.c
src/modules/actions/generic.py
src/modules/lint/pkglint_action.py
src/pkg/transforms/defaults
src/tests/api/t_action.py
src/tests/api/t_pkglint.py
--- a/doc/guide-metadata-conventions.rst	Mon Jul 11 12:30:28 2011 -0700
+++ b/doc/guide-metadata-conventions.rst	Mon Jul 11 16:34:01 2011 -0700
@@ -106,9 +106,9 @@
        A descriptive paragraph for the package.  Exact numerical version
        strings can be embedded in the paragraph.
 
-    pkg.detailed_url
+    pkg.detailed-url
        One or more URLs to pages or sites with further information about
-       the package. pkg.detailed_url:fr would be the URL to a page with
+       the package. pkg.detailed-url:fr would be the URL to a page with
        information in French.
 
     pkg.renamed
@@ -170,7 +170,7 @@
     package.  For an individual, this string is expected to be their
     name, or name and email.
 
-info.maintainer_url
+info.maintainer-url
     A URL associated with the entity providing the package.
 
 info.upstream
@@ -178,20 +178,20 @@
     software.  For an individual, this string is expected to be
     their name, or name and email.
 
-info.upstream_url
+info.upstream-url
     A URL associated with the entity that creates the 
     software delivered within the package.
 
-info.source_url
+info.source-url
     A URL to the source code bundle, if appropriate, for the package.
 
-info.repository_url
+info.repository-url
     A URL to the source code repository, if appropriate, for the
     package.
 
-info.repository_changeset
+info.repository-changeset
     A changeset ID for the version of the source code contained in
-    info.repository_url.
+    info.repository-url.
 
 Attributes common to all packages for an OS platform
 ````````````````````````````````````````````````````
@@ -203,9 +203,10 @@
 OpenSolaris attributes
 ^^^^^^^^^^^^^^^^^^^^^^
 
-    opensolaris.arc_url
-        One or more URLs associated with the ARC case or cases
-	associated with the component(s) delivered by the package.
+    org.opensolaris.arc-caseid
+        One or more case identifiers (e.g., PSARC/2008/190) associated with
+        the ARC case or cases associated with the component(s) delivered by
+        the package.
 
     org.opensolaris.smf.fmri
 	One or more FMRI's representing SMF services delivered by this
@@ -234,8 +235,8 @@
     or to amend an existing package's metadata (in a repository that
     they have control over) must use an organization-specific prefix.
     For example, a service organization might introduce
-    ``service.example.com,support_level`` or
-    ``com.example.service,support_level`` to describe a level of support
+    ``service.example.com,support-level`` or
+    ``com.example.service,support-level`` to describe a level of support
     for a package and its contents.
 
 Attributes specific to certain types of actions
--- a/doc/tags-and-attributes.txt	Mon Jul 11 12:30:28 2011 -0700
+++ b/doc/tags-and-attributes.txt	Mon Jul 11 16:34:01 2011 -0700
@@ -77,9 +77,9 @@
        A descriptive paragraph for the package.  Exact numerical version
        strings can be embedded in the paragraph.
 
-    pkg.detailed_url
+    pkg.detailed-url
        One or more URLs to pages or sites with further information about
-       the package.  pkg.detailed_url:fr would be the URL to a page with
+       the package.  pkg.detailed-url:fr would be the URL to a page with
        information in French.
 
     pkg.renamed
@@ -139,7 +139,7 @@
        package.  For an individual, this string is expected to be their
        name, or name and email.
 
-    info.maintainer_url
+    info.maintainer-url
        A URL associated with the entity providing the package.
 
     info.upstream
@@ -147,20 +147,20 @@
        software.  For an individual, this string is expected to be
        their name, or name and email.
 
-    info.upstream_url
+    info.upstream-url
        A URL associated with the entity that creates the 
        software delivered within the package.
 
-    info.source_url
+    info.source-url
        A URL to the source code bundle, if appropriate, for the package.
 
-    info.repository_url
+    info.repository-url
        A URL to the source code repository, if appropriate, for the
        package.
 
-    info.repository_changeset
+    info.repository-changeset
        A changeset ID for the version of the source code contained in
-       info.repository_url.
+       info.repository-url.
 
 
 2.4.  Attributes & tags common to all packages for an OS platform
@@ -171,9 +171,10 @@
 
 2.4.1.  OpenSolaris attributes
 
-    opensolaris.arc_url
-        One or more URLs associated with the ARC case or cases
-	associated with the component(s) delivered by the package.
+    org.opensolaris.arc-caseid
+	One or more case identifiers (e.g., PSARC/2008/190) associated with
+	the ARC case or cases associated with the component(s) delivered by
+	the package.
 
     org.opensolaris.smf.fmri
 	One or more FMRI's representing SMF services delivered by this
@@ -200,8 +201,8 @@
     or to amend an existing package's metadata (in a repository that
     they have control over) must use an organization-specific prefix.
     For example, a service organization might introduce
-    "service.example.com,support_level" or
-    "com.example.service,support_level" to describe a level of support
+    "service.example.com,support-level" or
+    "com.example.service,support-level" to describe a level of support
     for a package and its contents.
 
 2.5.1   Sun/Oracle attributes
@@ -210,7 +211,7 @@
     the OpenSolaris /support repository and are subject to change as 
     Sun integrates into Oracle.
 
-    com.sun.service.incorporated_changes
+    com.sun.service.incorporated-changes
 	A space-separated list of Change Requests ids in the Sun bugtraq 
 	database for the changes incorporated in this revision of the 
 	package that were not in the previous revision on the same branch.
--- a/src/client.py	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/client.py	Mon Jul 11 16:34:01 2011 -0700
@@ -3154,7 +3154,7 @@
                                 a = action.attrs[action.key_attr]
                         elif attr == "action.raw":
                                 a = action
-                        elif attr == "action.hash":
+                        elif attr in ("hash", "action.hash"):
                                 a = getattr(action, "hash", "")
                         elif attr == "pkg.name":
                                 a = pfmri.get_name()
--- a/src/man/pkg.5.txt	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/man/pkg.5.txt	Mon Jul 11 16:34:01 2011 -0700
@@ -86,6 +86,27 @@
      packaging system, others may be useful only to the system
      administrator or the end-user.
 
+     Actions have a simple text representation:
+
+         <action name> <attribute1>=<value1> <attribute2>=<value2> ...
+
+     Names of attributes cannot have whitespace, quotes, or equals signs
+     ("=") in them -- all characters after the first equals sign belong to
+     the value.  Values may have all of those, though spaces must be
+     enclosed in single or double quotes.  Single quotes need not be
+     escaped inside of a double-quoted string, and vice versa, though a
+     quote may be prefixed with a backslash ("\") so as not to terminate
+     the quoted string.  Backslashes may be escaped with backslashes.
+
+     Attributes may be named more than once with multiple values; these are
+     treated as unordered lists.
+
+     Attributes with many attributes may create long lines in a manifest
+     file.  Such lines can be wrapped by terminating each incomplete line
+     with a backslash.  Note that this continuation character must occur
+     between attribute/value pairs; neither attributes nor their values nor
+     the combination may be split.
+
      The attributes listed below are not an exhaustive set; indeed, the
      attributes which may be attached to an action are arbitrary, and
      indeed, the standard sets are easily augmented to incorporate future
@@ -110,6 +131,15 @@
 
      group  The name of the group which owns the file.
 
+     The payload is a "positional" attribute, in that it is not named.  It
+     is the first word after the action name.  In a published manifest, it
+     is the SHA-1 hash of the file contents.  If present in a manifest that
+     has yet to be published, it represents the path at which the payload
+     may be found (see pkgsend(1)).  The "hash" attribute may be used
+     instead of the positional attribute, should the value include an
+     equals sign.  Both may be used in the same action; however, the hashes
+     must be identical.
+
      Other attributes include:
 
      preserve  This specifies that the file's contents should not be
--- a/src/modules/actions/_actions.c	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/modules/actions/_actions.c	Mon Jul 11 16:34:01 2011 -0700
@@ -30,6 +30,8 @@
 static PyObject *MalformedActionError;
 static PyObject *InvalidActionError;
 
+static char *notident = "hash attribute not identical to positional hash";
+
 static int
 add_to_attrs(PyObject *attrs, PyObject *key, PyObject *attr)
 {
@@ -91,6 +93,7 @@
 {
 	char *s = NULL;
 	char *str = NULL;
+	char *hashstr = NULL;
 	char *keystr = NULL;
 	int *slashmap = NULL;
 	int strl;
@@ -124,7 +127,8 @@
 	Py_XDECREF(type);\
 	Py_XDECREF(attr);\
 	Py_XDECREF(attrs);\
-	Py_XDECREF(hash);
+	Py_XDECREF(hash);\
+	free(hashstr);
 
 	/*
 	 * The action string is currently assumed to be a stream of bytes that
@@ -177,6 +181,7 @@
 						CLEANUP_REFS;
 						return (NULL);
 					}
+					hashstr = strndup(keystr, keysize);
 					state = WS;
 				}
 			} else if (str[i] == '=') {
@@ -314,10 +319,21 @@
 					}
 				}
 
-				PyString_InternInPlace(&attr);
-				if (add_to_attrs(attrs, key, attr) == -1) {
-					CLEANUP_REFS;
-					return (NULL);
+				if (!strncmp(keystr, "hash=", 5)) {
+					char *as = PyString_AsString(attr);
+					if (hashstr && strcmp(as, hashstr)) {
+						invalid(notident);
+						CLEANUP_REFS;
+						return (NULL);
+					}
+					hash = attr;
+					attr = NULL;
+				} else {
+					PyString_InternInPlace(&attr);
+					if (add_to_attrs(attrs, key, attr) == -1) {
+						CLEANUP_REFS;
+						return (NULL);
+					}
 				}
 			}
 		} else if (state == UQVAL) {
@@ -325,10 +341,21 @@
 				state = WS;
 				Py_XDECREF(attr);
 				attr = PyString_FromStringAndSize(&str[vs], i - vs);
-				PyString_InternInPlace(&attr);
-				if (add_to_attrs(attrs, key, attr) == -1) {
-					CLEANUP_REFS;
-					return (NULL);
+				if (!strncmp(keystr, "hash=", 5)) {
+					char *as = PyString_AsString(attr);
+					if (hashstr && strcmp(as, hashstr)) {
+						invalid(notident);
+						CLEANUP_REFS;
+						return (NULL);
+					}
+					hash = attr;
+					attr = NULL;
+				} else {
+					PyString_InternInPlace(&attr);
+					if (add_to_attrs(attrs, key, attr) == -1) {
+						CLEANUP_REFS;
+						return (NULL);
+					}
 				}
 			}
 		} else if (state == WS) {
@@ -361,10 +388,21 @@
 	if (state == UQVAL) {
 		Py_XDECREF(attr);
 		attr = PyString_FromStringAndSize(&str[vs], i - vs);
-		PyString_InternInPlace(&attr);
-		if (add_to_attrs(attrs, key, attr) == -1) {
-			CLEANUP_REFS;
-			return (NULL);
+		if (!strncmp(keystr, "hash=", 5)) {
+			char *as = PyString_AsString(attr);
+			if (hashstr && strcmp(as, hashstr)) {
+				invalid(notident);
+				CLEANUP_REFS;
+				return (NULL);
+			}
+			hash = attr;
+			attr = NULL;
+		} else {
+			PyString_InternInPlace(&attr);
+			if (add_to_attrs(attrs, key, attr) == -1) {
+				CLEANUP_REFS;
+				return (NULL);
+			}
 		}
 	}
 
--- a/src/modules/actions/generic.py	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/modules/actions/generic.py	Mon Jul 11 16:34:01 2011 -0700
@@ -305,7 +305,10 @@
 
                 out = self.name
                 if hasattr(self, "hash") and self.hash is not None:
-                        out += " " + self.hash
+                        if "=" not in self.hash:
+                                out += " " + self.hash
+                        else:
+                                self.attrs["hash"] = self.hash
 
                 def q(s):
                         if " " in s or "'" in s or "\"" in s or s == "":
@@ -335,6 +338,11 @@
                                         out += " " + k + "=\"" + v.replace("\"", "\\\"") + "\""
                         else:
                                 out += " " + k + "=" + v
+
+                # If we have a hash attribute, it's because we added it above;
+                # get rid of it now.
+                self.attrs.pop("hash", None)
+
                 return out
 
         def __repr__(self):
--- a/src/modules/lint/pkglint_action.py	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/modules/lint/pkglint_action.py	Mon Jul 11 16:34:01 2011 -0700
@@ -590,10 +590,29 @@
 
                 name = action.attrs["name"]
 
-                if "_" not in name or name in ["info.maintainer_url",
-                    "info.upstream_url", "info.source_url",
-                    "info.repository_url", "info.repository_changeset",
-                    "info.defect_tracker.url", "opensolaris.arc_url"]:
+                if "_" not in name:
+                        return
+
+                obs_map = {
+                    "info.maintainer_url": "info.maintainer-url",
+                    "info.upstream_url": "info.upstream-url",
+                    "info.source_url": "info.source-url",
+                    "info.repository_url": "info.repository-url",
+                    "info.repository_changeset": "info.repository-changeset",
+                    "info.defect_tracker.url": "info.defect-tracker.url",
+                    "opensolaris.arc_url": "org.opensolaris.caseid"
+                }
+
+                # These names are deprecated, and so we warn, but we're a tiny
+                # bit nicer about it.
+                if name in obs_map:
+                        engine.warning(_("underscore in obsolete 'set' action "
+                            "name %(name)s should be %(new)s in %(fmri)s") % {
+                                "name": name,
+                                "new": obs_map[name],
+                                "fmri": manifest.fmri
+                            },
+                            msgid="%s%s.3" % (self.name, pkglint_id))
                         return
 
                 engine.warning(_("underscore in 'set' action name %(name)s in "
--- a/src/pkg/transforms/defaults	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/pkg/transforms/defaults	Mon Jul 11 16:34:01 2011 -0700
@@ -93,16 +93,16 @@
 # add a ,REV= string to any version attributes that don't have a REV=
 <transform legacy -> edit version (^(?!.*?,REV=).*) \\1,REV=$(REV)>
 
-# only set the info.repository_* values for packages we're responsible for
-<transform pkg pkg.fmri=pkg:/package/.* -> default info.repository_changeset $(CHANGESET)>
-<transform pkg pkg.fmri=pkg:/package/.* -> default info.repository_url ssh://hg.opensolaris.org/hg/pkg/gate>
-<transform pkg pkg.fmri=pkg:/system/zones.*-> default info.repository_changeset $(CHANGESET)>
-<transform pkg pkg.fmri=pkg:/system/zones.* -> default info.repository_url ssh://hg.opensolaris.org/hg/pkg/gate>
-<transform pkg pkg.fmri=pkg:/developer/opensolaris/pkg5 -> default info.repository_changeset $(CHANGESET)>
-<transform pkg pkg.fmri=pkg:/developer/opensolaris/pkg5 -> default info.repository_url ssh://hg.opensolaris.org/hg/pkg/gate>
+# only set the info.repository-* values for packages we're responsible for
+<transform pkg pkg.fmri=pkg:/package/.* -> default info.repository-changeset $(CHANGESET)>
+<transform pkg pkg.fmri=pkg:/package/.* -> default info.repository-url ssh://hg.opensolaris.org/hg/pkg/gate>
+<transform pkg pkg.fmri=pkg:/system/zones.*-> default info.repository-changeset $(CHANGESET)>
+<transform pkg pkg.fmri=pkg:/system/zones.* -> default info.repository-url ssh://hg.opensolaris.org/hg/pkg/gate>
+<transform pkg pkg.fmri=pkg:/developer/opensolaris/pkg5 -> default info.repository-changeset $(CHANGESET)>
+<transform pkg pkg.fmri=pkg:/developer/opensolaris/pkg5 -> default info.repository-url ssh://hg.opensolaris.org/hg/pkg/gate>
 # now emit those values
-<transform pkg info.repository_changeset=(.*) -> emit set name=info.repository_changeset value=%<1>>
-<transform pkg info.repository_url=(.*) -> emit set name=info.repository_url value=%<1>>
+<transform pkg info.repository-changeset=(.*) -> emit set name=info.repository-changeset value=%<1>>
+<transform pkg info.repository-url=(.*) -> emit set name=info.repository-url value=%<1>>
 
 # vim: ft=pkg5manifest
 
--- a/src/tests/api/t_action.py	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/tests/api/t_action.py	Mon Jul 11 16:34:01 2011 -0700
@@ -216,6 +216,11 @@
                 a = action.fromstr(r'set name=pkg.description value="Sphinx is a tool that makes it easy to create intelligent \"and beautiful documentation f\"or Python projects (or \"other documents consisting of\" multiple reStructuredText so\"urces), written by Georg Bran\"dl. It was originally created\" to translate the new Python \"documentation, but has now be\"en cleaned up in the hope tha\"t it will be useful to many o\"ther projects. Sphinx uses re\"StructuredText as its markup \"language, and many of its str\"engths come from the power an\"d straightforwardness of reSt\"ructuredText and its parsing \"and translating suite, the Do\"cutils. Although it is still \"under constant development, t\"he following features are alr\"eady present, work fine and c\"an be seen \"in action\" \"in the Python docs: * Output \"formats: HTML (including Wind\"ows HTML Help) and LaTeX, for\" printable PDF versions * Ext\"ensive cross-references: sema\"ntic markup and automatic lin\"ks for functions, classes, gl\"ossary terms and similar piec\"es of information * Hierarchi\"cal structure: easy definitio\"n of a document tree, with au\"tomatic links to siblings, pa\"rents and children * Automati\"c indices: general index as w\"ell as a module index * Code \"handling: automatic highlight\"ing using the Pygments highli\"ghter * Various extensions ar\"e available, e.g. for automat\"ic testing of snippets and in\"clusion of appropriately formatted docstrings."')
                 self.assert_(a.attrs["value"].count('"') == 45)
 
+                # Make sure that the hash member of the action object properly
+                # contains the value of the "hash" named attribute.
+                a = action.fromstr("file hash=abc123 path=usr/bin/foo mode=0755 owner=root group=bin")
+                self.assert_(a.hash == "abc123")
+
         def test_action_license(self):
                 """Test license action attributes."""
 
@@ -259,6 +264,24 @@
                                 self.debug("a2 " + str(a2))
                                 self.assert_(not a.different(a2))
 
+                # The one place that invariant doesn't hold is when you specify
+                # the payload hash as the named attribute "hash", in which case
+                # the resulting re-stringification emits the payload hash as a
+                # positional attribute again ...
+                s = "file hash=abc123 path=usr/bin/foo mode=0755 owner=root group=bin"
+                self.debug(s)
+                a = action.fromstr(s)
+                s2 = str(a)
+                self.assert_(s2.startswith("file abc123 "))
+                self.assert_("hash=abc123" not in s2)
+                a2 = action.fromstr(s2)
+                self.assert_(not a.different(a2))
+
+                # ... unless of course the hash can't be represented that way.
+                a = action.fromstr("file hash=abc=123 path=usr/bin/foo mode=0755 owner=root group=bin")
+                self.assert_("hash=abc=123" in str(a))
+                self.assert_(not str(a).startswith("file abc=123"))
+
         def test_action_sig_str(self):
                 sig_act = action.fromstr(
                     "signature 54321 algorithm=baz")
@@ -386,6 +409,11 @@
                 self.assertRaises(action.InvalidActionError, action.fromstr,
                     "signature 12345 pkg.cert=bar")
 
+                # The payload hash can't be specified as both a named and a
+                # positional attribute if they're not identical.
+                self.assertInvalid("file xyz789 hash=abc123 path=usr/bin/foo mode=0755 owner=root group=bin")
+                action.fromstr("file abc123 hash=abc123 path=usr/bin/foo mode=0755 owner=root group=bin")
+
         def test_validate(self):
                 """Verify that action validate() works as expected; currently
                 only used during publication or action execution failure."""
--- a/src/tests/api/t_pkglint.py	Mon Jul 11 12:30:28 2011 -0700
+++ b/src/tests/api/t_pkglint.py	Mon Jul 11 16:34:01 2011 -0700
@@ -1124,7 +1124,7 @@
 """
 
 expected_failures["underscores.mf"] = ["pkglint.action001.1",
-    "pkglint.action001.2"]
+    "pkglint.action001.3", "pkglint.action001.2"]
 broken_manifests["underscores.mf"] = \
 """
 #