[1/5] support: don't pass to set_search_path a big struct by value
Commit Message
Fixes LGTM warning: " This parameter of type resolv_redirect_config is
88 bytes - consider passing a const pointer/reference instead."
Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>
---
support/resolv_test.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Comments
* Konstantin Kharlamov:
> /* Copy the search path from CONFIG.search to the _res object. */
> static void
> -set_search_path (struct resolv_redirect_config config)
> +set_search_path (const struct resolv_redirect_config* config)
This static function is called once and therefore inlined, so the
change does not matter.
It matters in terms of the code as read by human: α) one would no
longer wonder why "by-value" is used instead of a "by-pointer", and β)
the warning by static analyzer ceases to exist.
-----
Thanks for comments! I'll address the other comments (one about testing
suite + one patch changes ABI, have to be dropped), likely, on the
weekend, and will resend v2 of the series.
On Пн, Mar 25, 2019 at 09:38, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Konstantin Kharlamov:
>
>> /* Copy the search path from CONFIG.search to the _res object. */
>> static void
>> -set_search_path (struct resolv_redirect_config config)
>> +set_search_path (const struct resolv_redirect_config* config)
>
> This static function is called once and therefore inlined, so the
> change does not matter.
* Konstantin Kharlamov:
> It matters in terms of the code as read by human: α) one would no
> longer wonder why "by-value" is used instead of a "by-pointer",
With by-pointer, the reader has to wonder if the called function
retains a pointer after the call, so it is a wash in this regard.
On Ср, Mar 27, 2019 at 09:56, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Konstantin Kharlamov:
>
>> It matters in terms of the code as read by human: α) one would no
>> longer wonder why "by-value" is used instead of a "by-pointer",
>
> With by-pointer, the reader has to wonder if the called function
> retains a pointer after the call, so it is a wash in this regard.
Sorry, what you mean by "retains"? As in, "stores into a static
variable for caching purposes"? Well, first of, if a struct contains
pointers, then it doesn't even matter whether it was passed by-value or
by-pointer — one could get a data race either way. Second, unless the
function being explicit about caching a parameter, doing that behind
the curtains sounds outright wrong as it gonna lead to hard to debug
bugs.
So, normally I would not assume that an arbitrary function could retain
its argument.
@@ -1079,7 +1079,7 @@ resolv_test_init (void)
/* Copy the search path from CONFIG.search to the _res object. */
static void
-set_search_path (struct resolv_redirect_config config)
+set_search_path (const struct resolv_redirect_config* config)
{
memset (_res.defdname, 0, sizeof (_res.defdname));
memset (_res.dnsrch, 0, sizeof (_res.dnsrch));
@@ -1088,15 +1088,15 @@ set_search_path (struct resolv_redirect_config config)
char *end = current + sizeof (_res.defdname);
for (unsigned int i = 0;
- i < sizeof (config.search) / sizeof (config.search[0]); ++i)
+ i < sizeof (config->search) / sizeof (config->search[0]); ++i)
{
- if (config.search[i] == NULL)
+ if (config->search[i] == NULL)
continue;
- size_t length = strlen (config.search[i]) + 1;
+ size_t length = strlen (config->search[i]) + 1;
size_t remaining = end - current;
TEST_VERIFY_EXIT (length <= remaining);
- memcpy (current, config.search[i], length);
+ memcpy (current, config->search[i], length);
_res.dnsrch[i] = current;
current += length;
}
@@ -1203,7 +1203,7 @@ resolv_test_start (struct resolv_redirect_config config)
}
}
- set_search_path (config);
+ set_search_path (&config);
return obj;
}