Re: [Rpm-maint] [PATCH] Use a file list to add build-id files to pkgList and explicitly set attrs.

[Panu Matilainen]

On 06/22/2017 04:31 PM, Mark Wielaard wrote:

Hi,

Panu sadi on irc he didn't like the duplication of code that parsed the
spec file lists. So this updated patch extracts the setup and parsing
loop in their own function and just calls them twice. I also reformatted
the patch a little so the whitespace differences are minimal.



Hey, sorry for not responding to this earlier, been on PTOs on and off 
and between them just just forgot the whole thing.


For whatever reason the patch is was attached instead of inline so 
harder to comment, but I think we can make do:


Please do the refactor to helper function(s) in a separate patch from 
the rest of the changes, it'll be easier to review and bisect too if it 
ever comes to that.


mkattr() with non-NULL fn argument ceases to be meaningful here, and 
since it's not even used for anything, whether the mode should be 644 or 
755 nobody knows, certainly not that function. Better just drop 
non-defattr case from it entirely.


Finally, I've no particular objections to changing it this way, but it's 
not as obvious win (ends up actually being more code) as at least I 
initially thought. I've attached (apologies for dumb email clients) a 
more minimal version that should do just the same: enforce a sane 
%defattr across the generated files and directories. Might be nicer for 
Fedora backport if nothing else.


- Panu -


diff --git a/build/files.c b/build/files.c
index 4911162..df53d1d 100644
--- a/build/files.c
+++ b/build/files.c
@@ -206,10 +206,7 @@ static void dupAttrRec(const AttrRec oar, AttrRec nar)
 static char *mkattr(const char *fn)
 {
 char *s = NULL;
-if (fn)
-	rasprintf(, "%s(-,%s,%s) %s", "%attr", UID_0_USER, GID_0_GROUP, fn);
-else
-	rasprintf(, "%s(-,%s,%s)", "%defattr", UID_0_USER, GID_0_GROUP);
+rasprintf(, "%s(644,%s,%s,755)", "%defattr", UID_0_USER, GID_0_GROUP);
 return s;
 }
 
@@ -1881,11 +1878,8 @@ static int generateBuildIDs(FileList fl)
 		if ((rc = rpmioMkpath(mainiddir, 0755, -1, -1)) != 0) {
 		rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, mainiddir);
 		} else {
-		attrstr = mkattr(mainiddir);
-		parseForAttr(fl->pool, attrstr, 0, >cur);
 		fl->cur.isDir = 1;
 		rc = addFile(fl, mainiddir, NULL);
-		free (attrstr);
 		}
 	}
 
@@ -1893,11 +1887,8 @@ static int generateBuildIDs(FileList fl)
 		if ((rc = rpmioMkpath(debugiddir, 0755, -1, -1)) != 0) {
 		rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, debugiddir);
 		} else {
-		attrstr = mkattr(debugiddir);
-		parseForAttr(fl->pool, attrstr, 0, >cur);
 		fl->cur.isDir = 1;
 		rc = addFile(fl, debugiddir, NULL);
-		free (attrstr);
 		}
 	}
 	}
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [PATCH] Use a file list to add build-id files to pkgList and explicitly set attrs.

[Mark Wielaard]

Hi,

Panu sadi on irc he didn't like the duplication of code that parsed the
spec file lists. So this updated patch extracts the setup and parsing
loop in their own function and just calls them twice. I also reformatted
the patch a little so the whitespace differences are minimal.

Cheers,

Mark

From 739796798ac854f80ae2f0d677f74bca734055f7 Mon Sep 17 00:00:00 2001
From: Mark Wielaard 
Date: Wed, 21 Jun 2017 16:57:13 +0200
Subject: [PATCH] Use a file list to add build-id files to pkgList and
 explicitly set attrs.

mkattr used "-" as default mode which would pick up the mode for files
as they were on disk. This could cause files generated by rpmbuild to
use a "non-standard" mode if umask was set by the user. Explitictly
use 755 for directories and 644 for files to make builds independent
of any umask settings.

Change the generation of build-id files to a file list using ARGV_t.
First go through the current package list and generate a files list.
Then add those files using the defaults mode/attr settings as if they
were part of the original package file list.

https://bugzilla.redhat.com/show_bug.cgi?id=1452893
https://bugzilla.redhat.com/show_bug.cgi?id=1458839

Signed-off-by: Mark Wielaard 
---
 build/files.c | 244 +-
 1 file changed, 138 insertions(+), 106 deletions(-)

diff --git a/build/files.c b/build/files.c
index 4911162d1..a90af3bee 100644
--- a/build/files.c
+++ b/build/files.c
@@ -207,9 +207,9 @@ static char *mkattr(const char *fn)
 {
 char *s = NULL;
 if (fn)
-	rasprintf(, "%s(-,%s,%s) %s", "%attr", UID_0_USER, GID_0_GROUP, fn);
+	rasprintf(, "%s(755,%s,%s) %s", "%attr", UID_0_USER, GID_0_GROUP, fn);
 else
-	rasprintf(, "%s(-,%s,%s)", "%defattr", UID_0_USER, GID_0_GROUP);
+	rasprintf(, "%s(644,%s,%s,755)", "%defattr", UID_0_USER, GID_0_GROUP);
 return s;
 }
 
@@ -1614,6 +1614,15 @@ exit:
 return rc;
 }
 
+/* add a directory to the file list */
+static void argvAddDir(ARGV_t *filesp, const char *dir)
+{
+char *line = NULL;
+rasprintf(, "%%dir %s", dir);
+argvAdd(filesp, line);
+_free(line);
+}
+
 #if HAVE_LIBDW
 /* How build id links are generated.  See macros.in for description.  */
 #define BUILD_IDS_NONE 0
@@ -1621,7 +1630,7 @@ exit:
 #define BUILD_IDS_SEPARATE 2
 #define BUILD_IDS_COMPAT   3
 
-static int addNewIDSymlink(FileList fl,
+static int addNewIDSymlink(ARGV_t *files,
 			   char *targetpath, char *idlinkpath,
 			   int isDbg, int *dups)
 {
@@ -1670,8 +1679,7 @@ static int addNewIDSymlink(FileList fl,
 	rpmlog(RPMLOG_ERR, "%s: %s -> %s: %m\n",
 	   linkerr, linkpath, targetpath);
 } else {
-	fl->cur.isDir = 0;
-	rc = addFile(fl, linkpath, NULL);
+	rc = argvAdd(files, linkpath);
 }
 
 if (nr > 0) {
@@ -1709,7 +1717,7 @@ static int addNewIDSymlink(FileList fl,
 return rc;
 }
 
-static int generateBuildIDs(FileList fl)
+static int generateBuildIDs(FileList fl, ARGV_t *files)
 {
 int rc = 0;
 int i;
@@ -1858,18 +1866,9 @@ static int generateBuildIDs(FileList fl)
 	mainiddir = rpmGetPath(fl->buildRoot, BUILD_ID_DIR, NULL);
 	debugiddir = rpmGetPath(fl->buildRoot, DEBUG_ID_DIR, NULL);
 
-	/* Make sure to reset all file flags to defaults.
-	   Uses parseForAttr to reset ar, arFlags, and specdFlags.
-	   Note that parseForAttr pokes at the attrstr, so we cannot
-	   just pass a static string. */
-	fl->cur.attrFlags = 0;
-	fl->def.attrFlags = 0;
-	fl->def.verifyFlags = RPMVERIFY_ALL;
-	fl->cur.verifyFlags = RPMVERIFY_ALL;
-	fl->def.specdFlags |= SPECD_VERIFY;
-	fl->cur.specdFlags |= SPECD_VERIFY;
+	/* Make sure to reset all file flags to defaults.  */
 	attrstr = mkattr(NULL);
-	parseForAttr(fl->pool, attrstr, 1, >def);
+	argvAdd(files, attrstr);
 	free (attrstr);
 
 	/* Supported, but questionable.  */
@@ -1881,11 +1880,7 @@ static int generateBuildIDs(FileList fl)
 		if ((rc = rpmioMkpath(mainiddir, 0755, -1, -1)) != 0) {
 		rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, mainiddir);
 		} else {
-		attrstr = mkattr(mainiddir);
-		parseForAttr(fl->pool, attrstr, 0, >cur);
-		fl->cur.isDir = 1;
-		rc = addFile(fl, mainiddir, NULL);
-		free (attrstr);
+		argvAddDir(files, mainiddir);
 		}
 	}
 
@@ -1893,11 +1888,7 @@ static int generateBuildIDs(FileList fl)
 		if ((rc = rpmioMkpath(debugiddir, 0755, -1, -1)) != 0) {
 		rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, debugiddir);
 		} else {
-		attrstr = mkattr(debugiddir);
-		parseForAttr(fl->pool, attrstr, 0, >cur);
-		fl->cur.isDir = 1;
-		rc = addFile(fl, debugiddir, NULL);
-		free (attrstr);
+		argvAddDir(files, debugiddir);
 		}
 	}
 	}
@@ -1936,9 +1927,9 @@ static int generateBuildIDs(FileList fl)
 		&& (rc = rpmioMkpath(buildidsubdir, 0755, -1, -1)) != 0) {
 		rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, buildidsubdir);
 		} else {
-		fl->cur.isDir = 

[Rpm-maint] [PATCH] Use a file list to add build-id files to pkgList and explicitly set attrs.

[Mark Wielaard]

When setting attributes with %attr or %defattr we want to be explicit
about the mode and not just use - to pick up the mode from the file
on disk.

Change the generation of build-id files to a file list using ARGV_t.
First go through the current package list and generate a files list.
Then add those files using the defaults mode/attr settings as if they
were part of the original package file list.

https://bugzilla.redhat.com/show_bug.cgi?id=1452893
https://bugzilla.redhat.com/show_bug.cgi?id=1458839

Signed-off-by: Mark Wielaard 
---
 build/files.c | 310 +++---
 1 file changed, 167 insertions(+), 143 deletions(-)

diff --git a/build/files.c b/build/files.c
index 4911162..be72f0e 100644
--- a/build/files.c
+++ b/build/files.c
@@ -207,9 +207,9 @@ static char *mkattr(const char *fn)
 {
 char *s = NULL;
 if (fn)
-   rasprintf(, "%s(-,%s,%s) %s", "%attr", UID_0_USER, GID_0_GROUP, fn);
+   rasprintf(, "%s(755,%s,%s) %s", "%attr", UID_0_USER, GID_0_GROUP, fn);
 else
-   rasprintf(, "%s(-,%s,%s)", "%defattr", UID_0_USER, GID_0_GROUP);
+   rasprintf(, "%s(644,%s,%s,755)", "%defattr", UID_0_USER, GID_0_GROUP);
 return s;
 }
 
@@ -1614,6 +1614,15 @@ exit:
 return rc;
 }
 
+/* add a directory to the file list */
+static void argvAddDir(ARGV_t *filesp, const char *dir)
+{
+char *line = NULL;
+rasprintf(, "%%dir %s", dir);
+argvAdd(filesp, line);
+_free(line);
+}
+
 #if HAVE_LIBDW
 /* How build id links are generated.  See macros.in for description.  */
 #define BUILD_IDS_NONE 0
@@ -1621,7 +1630,7 @@ exit:
 #define BUILD_IDS_SEPARATE 2
 #define BUILD_IDS_COMPAT   3
 
-static int addNewIDSymlink(FileList fl,
+static int addNewIDSymlink(ARGV_t *files,
   char *targetpath, char *idlinkpath,
   int isDbg, int *dups)
 {
@@ -1670,8 +1679,7 @@ static int addNewIDSymlink(FileList fl,
rpmlog(RPMLOG_ERR, "%s: %s -> %s: %m\n",
   linkerr, linkpath, targetpath);
 } else {
-   fl->cur.isDir = 0;
-   rc = addFile(fl, linkpath, NULL);
+   rc = argvAdd(files, linkpath);
 }
 
 if (nr > 0) {
@@ -1709,7 +1717,7 @@ static int addNewIDSymlink(FileList fl,
 return rc;
 }
 
-static int generateBuildIDs(FileList fl)
+static int generateBuildIDs(FileList fl, ARGV_t *files)
 {
 int rc = 0;
 int i;
@@ -1858,18 +1866,9 @@ static int generateBuildIDs(FileList fl)
mainiddir = rpmGetPath(fl->buildRoot, BUILD_ID_DIR, NULL);
debugiddir = rpmGetPath(fl->buildRoot, DEBUG_ID_DIR, NULL);
 
-   /* Make sure to reset all file flags to defaults.
-  Uses parseForAttr to reset ar, arFlags, and specdFlags.
-  Note that parseForAttr pokes at the attrstr, so we cannot
-  just pass a static string. */
-   fl->cur.attrFlags = 0;
-   fl->def.attrFlags = 0;
-   fl->def.verifyFlags = RPMVERIFY_ALL;
-   fl->cur.verifyFlags = RPMVERIFY_ALL;
-   fl->def.specdFlags |= SPECD_VERIFY;
-   fl->cur.specdFlags |= SPECD_VERIFY;
+   /* Make sure to reset all file flags to defaults.  */
attrstr = mkattr(NULL);
-   parseForAttr(fl->pool, attrstr, 1, >def);
+   argvAdd(files, attrstr);
free (attrstr);
 
/* Supported, but questionable.  */
@@ -1881,11 +1880,7 @@ static int generateBuildIDs(FileList fl)
if ((rc = rpmioMkpath(mainiddir, 0755, -1, -1)) != 0) {
rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, mainiddir);
} else {
-   attrstr = mkattr(mainiddir);
-   parseForAttr(fl->pool, attrstr, 0, >cur);
-   fl->cur.isDir = 1;
-   rc = addFile(fl, mainiddir, NULL);
-   free (attrstr);
+   argvAddDir(files, mainiddir);
}
}
 
@@ -1893,11 +1888,7 @@ static int generateBuildIDs(FileList fl)
if ((rc = rpmioMkpath(debugiddir, 0755, -1, -1)) != 0) {
rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, debugiddir);
} else {
-   attrstr = mkattr(debugiddir);
-   parseForAttr(fl->pool, attrstr, 0, >cur);
-   fl->cur.isDir = 1;
-   rc = addFile(fl, debugiddir, NULL);
-   free (attrstr);
+   argvAddDir(files, debugiddir);
}
}
}
@@ -1936,114 +1927,113 @@ static int generateBuildIDs(FileList fl)
&& (rc = rpmioMkpath(buildidsubdir, 0755, -1, -1)) != 0) {
rpmlog(RPMLOG_ERR, "%s %s: %m\n", errdir, buildidsubdir);
} else {
-   fl->cur.isDir = 1;
-   if (!addsubdir
-   || (rc = addFile(fl, buildidsubdir, NULL)) == 0) {
-   char *linkpattern,