Re: [PATCHv2 2/2] rpm: run binary package generation via thread pools

[Alexander Kanavin <alexander.kanavin@linux.intel.com>]

On 06/08/2017 07:55 PM, Mark Hatle wrote:
>> +@@ -565,8 +559,8 @@ static rpmRC packageBinary(rpmSpec spec, Package pkg, const char *cookie, int ch
>> + 	headerCopyTags(spec->packages->header, pkg->header, copyTags);
>> + 	
>> + 	headerPutString(pkg->header, RPMTAG_RPMVERSION, VERSION);
>> +-	headerPutString(pkg->header, RPMTAG_BUILDHOST, buildHost());
>> +-	headerPutUint32(pkg->header, RPMTAG_BUILDTIME, getBuildTime(), 1);
>> ++	headerPutString(pkg->header, RPMTAG_BUILDHOST, buildHost);
>> ++	headerPutUint32(pkg->header, RPMTAG_BUILDTIME, &buildTime, 1);
>
> I don't see what the advantage of this change over the current approach is --
> unless the first call is in a thread so the variable can clash.  If that is the
> case, it may make more sense to ensure that these two are run very early in the
> build process before it threads.

They're run from threads, and they do clash, and I had spent a whole day 
tracing data corruption coredumps back to these two.

This is explained in the commit message:

"Their use is causing difficult to diagnoze data races when building 
multiple packages in parallel..."

And I had changed the code so that they're run early in the build 
process, and just once. See below...


>> +@@ -629,6 +623,8 @@ static struct binaryPackageTaskData* runBinaryPackageTasks(rpmSpec spec, const c
>> +     struct binaryPackageTaskData *tasks = NULL;
>> +     struct binaryPackageTaskData *task = NULL;
>> +     struct binaryPackageTaskData *prev = NULL;
>> ++    rpm_time_t buildTime = getBuildTime();
>> ++    char *host = buildHost();
>> +
>> +     for (Package pkg = spec->packages; pkg != NULL; pkg = pkg->next) {
>> +         task = rcalloc(1, sizeof(*task));


^^^^ right there.

Please read the code more carefully :-)


Alex