Re: [devel] [PATCH for apt 1/2] Add option for debugging DynamicMMap::Allocate

[Aleksei Nikiforov <darktemplar@altlinux.org>]

09.12.2019 1:50, Dmitry V. Levin пишет:
> On Fri, Dec 06, 2019 at 04:16:05PM +0300, Aleksei Nikiforov wrote:
>> ---
>>   apt/apt-pkg/contrib/mmap.cc | 15 +++++++++++++++
>>   apt/doc/apt.conf.5.sgml     |  2 +-
>>   2 files changed, 16 insertions(+), 1 deletion(-)
> 
> I suggest adding the name of the new option to the commit message
> so it would be git-grep'able, for example:
> 
> Add Debug::DynamicMMap::Allocate option
> 
> Add a new option for debugging DynamicMMap::Allocate.
> 

But option name is already in commit message. See the email subject 
which is also a part of commit message. Why is it needed to duplicate it?

I'll update option name in subject to completely match full option name.

>> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
>> index a3b06cc..cf01be9 100644
>> --- a/apt/apt-pkg/contrib/mmap.cc
>> +++ b/apt/apt-pkg/contrib/mmap.cc
>> @@ -265,6 +265,21 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>>      // No pool is allocated, use an unallocated one
>>      if (I == Pools + PoolCount)
>>      {
>> +      static const bool debug_grow = _config->FindB("Debug::DynamicMMap::Allocate", false);
>> +
>> +      if (debug_grow)
>> +      {
>> +         Pool *pool_iter = Pools;
>> +         size_t pool_idx = 0;
>> +
>> +         _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"), ItemSize);
>> +
>> +         for (; pool_idx < PoolCount; ++pool_iter, ++pool_idx)
>> +         {
>> +            _error->Warning(_("DynamicMMap::Allocate: Pool %zu, item size: %lu, start: %lu, count: %lu"), pool_idx, pool_iter->ItemSize, pool_iter->Start, pool_iter->Count);
>> +         }
>> +      }
> 
> Nitpicking:
> - this is debugging, there is no need to optimize, so there is no need for
>    pool_iter;
> - let's use "for" loop initial declarations, they are widely used in apt
>    already;
> - the type of iterator should be the same as the type of PoolCount;
> - lines are too long.
> 
> Consider something like this (untested):
> 
>        if (debug_grow) {
>           _error->Warning(_("DynamicMMap::Allocate: allocating item of size %lu"),
>                           ItemSize);
> 
>           for (unsigned int i = 0; i < PoolCount; ++i) {
>              _error->Warning(_("DynamicMMap::Allocate: Pool %u, item size: %lu"
>                                ", start: %lu, count: %lu"),
>                              i, Pools[i].ItemSize,
>                              Pools[i].Start, Pools[i].Count);
>           }
>        }
> 
>>         // Woops, we ran out, the calling code should allocate more.
>>         if (Empty == 0)
>>         {
>> diff --git a/apt/doc/apt.conf.5.sgml b/apt/doc/apt.conf.5.sgml
>> index 0a72e45..72fc0c3 100644
>> --- a/apt/doc/apt.conf.5.sgml
>> +++ b/apt/doc/apt.conf.5.sgml
>> @@ -526,7 +526,7 @@ DPkg::Pre-Install-Pkgs {"/usr/sbin/dpkg-preconfigure --apt";};
>>      disable the inclusion of statfs data in CDROM IDs.
>>      </para><para>
>>      To debug issues related to dynamic memory allocation, an option
>> -   <literal/Debug::DynamicMMap::Grow/ may be used.
>> +   <literal/Debug::DynamicMMap::Grow/ and <literal/Debug::DynamicMMap::Allocate/ may be used.
> 
> As we've got two options now, I suggest s/an option/options/ .
> 
> 

Thanks, I'll update this text.

> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel
>