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

["Dmitry V. Levin" <ldv@altlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 2978 bytes --]

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.

> 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/ .


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]