> On March 22, 2017, 2:45 p.m., Till Toenshoff wrote: > > src/linux/ldcache.cpp > > Line 120 (original), 120 (patched) > > <https://reviews.apache.org/r/55065/diff/1/?file=1592794#file1592794line120> > > > > Can you elaborate why this change is safe for all systems? > > Ayanampudi Varsha wrote: > We have tried this on Big endian system where 16 bit flag was causing > issue. > Test case LdcacheTest.Parse failed at condition ASSERT_GT(cache->size(), > 1u); as size was returned as 0. This happens because the 16 bits after the > field "flags" is unused. > On x86_64, I have verified that this "cache->size()" is same with 16 bit > and 32 bit flag. > Observed that the other fields in structure are all 32bit/64bit. > An alternative would be to make changes only specific to s390x by putting > a condition. Please suggest > > Till Toenshoff wrote: > You may actually have found a bug in our ldcache parsing. After looking > into the glibc sources, I think this field should have been 32bit also for > x86_64 instead of 16bit - I suspect missing struct-packing for this bug to > not have surfaced earlier. For confirming this without mixing it with your > platform specific docker-repository change, I would like you to split this > patch into two. > > So please add another patch that only fixes `flags` attribute size into > 32bit. Please make sure you add the following reviewers: tillt, klueska. > > Remove that 32bit change from this patch and update it according to my > and Kevin's review comments. > > Thanks a bunch for your work - this is highly appreciated. > > Till Toenshoff wrote: > Ow, for further reference of this `flags` change, here are the internal > glibc structures: > https://github.com/lattera/glibc/blob/master/sysdeps/generic/dl-cache.h#L80
As I am planning to drop the docker_registry customization change, can I resubmit removing docker_registry change in same patch and continue in the same review board ticket. please suggest - Ayanampudi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55065/#review169708 ----------------------------------------------------------- On March 27, 2017, 9:25 a.m., Ayanampudi Varsha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55065/ > ----------------------------------------------------------- > > (Updated March 27, 2017, 9:25 a.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-6742 > https://issues.apache.org/jira/browse/MESOS-6742 > > > Repository: mesos > > > Description > ------- > > Added support for s390x architecture > There are 2 issues addressed in this patch: > 1. LdcacheTest.Parse test case fails on s390x machines. > 2. From the value of flag docker_registry in slave/flags.cpp, amd64 images > get downloaded due to which test cases fail on s390x with "Exec format Error" > > > Diffs > ----- > > src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 > src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 > > > Diff: https://reviews.apache.org/r/55065/diff/1/ > > > Testing > ------- > > make check > make check GTEST_FILTER="LdcacheTest.Parse" > > > Thanks, > > Ayanampudi Varsha > >
