Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
Hi Junio,
On Tue, 30 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const
> > char *value, void *data)
> > opts->allow_ff = git_config_bool_or_int(key, value,
> > &error_flag);
> > else if (!strcmp(key, "options.mainline"))
> > opts->mainline = git_config_int(key, value);
> > - else if (!strcmp(key, "options.strategy"))
> > + else if (!strcmp(key, "options.strategy")) {
> > git_config_string(&opts->strategy, key, value);
> > - else if (!strcmp(key, "options.gpg-sign"))
> > + sequencer_entrust(opts, (char *) opts->strategy);
> > + }
> > + else if (!strcmp(key, "options.gpg-sign")) {
> > git_config_string(&opts->gpg_sign, key, value);
> > + sequencer_entrust(opts, (char *) opts->gpg_sign);
> > + }
> > else if (!strcmp(key, "options.strategy-option")) {
> > ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> > - opts->xopts[opts->xopts_nr++] = xstrdup(value);
> > + opts->xopts[opts->xopts_nr++] =
> > + sequencer_entrust(opts, xstrdup(value));
> > } else
> > return error(_("Invalid key: %s"), key);
>
> Hmm.
>
> I would have expected a call to sequencer_opts_clear(&opts) once the
> machinery is done with the options structure,
It is part of sequencer_remove_state().
> and among these places where an old value in opts->field is overwritten
> by a new one would get
>
> free(opt->field); opt->field = ... new value ...;
That is *exactly* what we *cannot* do.
You see, the replay_opts struct is *not necessarily* populated by
read_populate_opts(). So we really cannot tell whether we are free to
free(opt->field) or whether some other code still wants to reference that
memory in the end (or for that matter, whether it was malloc()ed).
That is the *precise* reason for the existence of sequencer_entrust().
Ciao,
Dscho
Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
Johannes Sixt writes: > Am 30.08.2016 um 19:52 schrieb Johannes Schindelin: >> Right, but that is exactly what I wanted to avoid, because it is rather >> inelegant to strdup() strings just so that we do not have to record what >> to free() and what not to free(). > > Please, excuse, but when I have to choose what is more "elegant": > > 1. strdup() sometimes so that I can later free() always > 2. use sequencer_entrust() > > I would choose 1. at all times. Let's not bring elegance or aesthetics in. It is like asking if garbage collected systems are elegant. I do not think it is a valid question. GC is a good pragmatic compromise after (1) declaring that keeping track of allocation is too cumbersome and programmers' time is better spent elsewhere, and (2) making sure hiccups caused by GC can be minimized to keep the latency down to acceptable levels. There are good compromises and bad ones. Does the proposed entrust() thing qualify as a good pragmatic compromise like GC does? > Particularly in this case: parsing options does not sound like a > major drain of resources, neither CPU- nor memory-wise. If entrust() does not have any CPU- or memory-cost, then comparing it with having to strdup() sometimes might be a useful comparison. But the entrust() thing has the allocation cost in order to track of what needs to be done at runtime, and just like strdup() needs to be done on things that are being borrowed, entrust() needs to be avoided on things that are being borrowed (and the caller needs to be sure not to entrust() the same piece of memory twice), so it does not reduce the cost on programmers' and readers' mental burden--the programmer always has to know which piece of memory is borrowed and which are owned by the subsystem. So...
Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
W dniu 30.08.2016 o 19:52, Johannes Schindelin pisze:
> Hi Kuba,
>
> On Tue, 30 Aug 2016, Jakub Narębski wrote:
>
>> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>>
>>> The sequencer reads options from disk and stores them in its struct
>>> for use during sequencer's operations.
>>>
>>> With this patch, the memory is released afterwards, plugging a
>>> memory leak.
>>>
>>> Signed-off-by: Johannes Schindelin
>>> ---
>>> sequencer.c | 13 ++---
>>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index b5be0f9..8d79091 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct
>>> replay_opts *opts)
>>> free(opts->owned[i]);
>>> free(opts->owned);
>>>
>>> + free(opts->xopts);
>>> +
>>
>> This looks like independent change, not related to using the
>> sequencer_entrust() to store options read from disk in replay_opts
>> struct to be able to free memory afterwards.
>>
>> I guess you wanted to avoid one line changes...
>
> Actually, it is not an independent change, but it free()s memory that has
> been allocated while reading the options, as the commit message says ;-)
>
>>> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const
>>> char *value, void *data)
>>
>> Sidenote: this patch would be easier to read if lines were reordered
>> as below, but I don't think any slider heuristics could help achieve
>> that automatically. Also, the patch might be invalid...
>>
>>> opts->allow_ff = git_config_bool_or_int(key, value,
>>> &error_flag);
>>> else if (!strcmp(key, "options.mainline"))
>>> opts->mainline = git_config_int(key, value);
>>> - else if (!strcmp(key, "options.strategy"))
>>> + else if (!strcmp(key, "options.strategy")) {
>>> git_config_string(&opts->strategy, key, value);
>>> + sequencer_entrust(opts, (char *) opts->strategy);
>>
>> I wonder if the ability to free strings dup-ed by git_config_string()
>> be something that is part of replay_opts, or rather remove_sequencer_state(),
>> that is a list of
>>
>> free(opts->strategy);
>> free(opts->gpg_sign);
>
> That is not necessarily possible because the way sequencer works, the
> options may have not actually be read from the file, but may be populated
> by the caller (in which case we do not necessarily want to require
> strdup()ing the strings just so that the sequencer can clean stuff up
> afterwards).
I guess from cursory browsing through the Git code that _currently_
they are only read from the config file, where git_config_string()
strdup's them, isn't it? And we want to prepare for the future, where
the caller would prepare replay_opts, and the caller would be responsible
for freeing data if necessary?
Would there be any sane situation where some of data should be owned
by caller (and freed by caller), and some of data should be owned by
sequencer library API (and freed in remove_sequencer_state())? If
not, perhaps *_entrust() mechanism is overthinking it, and we simply
need 'is_strdup' boolean flag or something like that...
>> The *_entrust() mechanism is more generic, but do we use this general-ness?
>> Well, it could be xstrdup or git_config_string doing entrust'ing...
>
> Right, but that is exactly what I wanted to avoid, because it is rather
> inelegant to strdup() strings just so that we do not have to record what
> to free() and what not to free().
Maybe inelegant, but it might be easier than inventing and implementing
*_entrust() mechanism, like Hannes wrote.
>
> BTW I have no objection at all to generalize this sequencer_entrust()
> mechanism further (read: to other, similar use cases), should it withstand
> the test of time.
Yeah, that's my take on it too.
--
Jakub Narębski
Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
Am 30.08.2016 um 19:52 schrieb Johannes Schindelin: Right, but that is exactly what I wanted to avoid, because it is rather inelegant to strdup() strings just so that we do not have to record what to free() and what not to free(). Please, excuse, but when I have to choose what is more "elegant": 1. strdup() sometimes so that I can later free() always 2. use sequencer_entrust() I would choose 1. at all times. Particularly in this case: parsing options does not sound like a major drain of resources, neither CPU- nor memory-wise. -- Hannes
Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
Johannes Schindelin writes:
> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char
> *value, void *data)
> opts->allow_ff = git_config_bool_or_int(key, value,
> &error_flag);
> else if (!strcmp(key, "options.mainline"))
> opts->mainline = git_config_int(key, value);
> - else if (!strcmp(key, "options.strategy"))
> + else if (!strcmp(key, "options.strategy")) {
> git_config_string(&opts->strategy, key, value);
> - else if (!strcmp(key, "options.gpg-sign"))
> + sequencer_entrust(opts, (char *) opts->strategy);
> + }
> + else if (!strcmp(key, "options.gpg-sign")) {
> git_config_string(&opts->gpg_sign, key, value);
> + sequencer_entrust(opts, (char *) opts->gpg_sign);
> + }
> else if (!strcmp(key, "options.strategy-option")) {
> ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> - opts->xopts[opts->xopts_nr++] = xstrdup(value);
> + opts->xopts[opts->xopts_nr++] =
> + sequencer_entrust(opts, xstrdup(value));
> } else
> return error(_("Invalid key: %s"), key);
Hmm.
I would have expected a call to sequencer_opts_clear(&opts) once the
machinery is done with the options structure, and among these places
where an old value in opts->field is overwritten by a new one would
get
free(opt->field); opt->field = ... new value ...;
Perhaps there was a good reason to do it this way (one valid reason
may be that there is _no_ good place to declare that opts is now
done and it is safe to call sequencer_opts_clear() on it), but this
looks backwards from the way things are usually done.
Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
Hi Kuba,
On Tue, 30 Aug 2016, Jakub Narębski wrote:
> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>
> > The sequencer reads options from disk and stores them in its struct
> > for use during sequencer's operations.
> >
> > With this patch, the memory is released afterwards, plugging a
> > memory leak.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > sequencer.c | 13 ++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index b5be0f9..8d79091 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct
> > replay_opts *opts)
> > free(opts->owned[i]);
> > free(opts->owned);
> >
> > + free(opts->xopts);
> > +
>
> This looks like independent change, not related to using the
> sequencer_entrust() to store options read from disk in replay_opts
> struct to be able to free memory afterwards.
>
> I guess you wanted to avoid one line changes...
Actually, it is not an independent change, but it free()s memory that has
been allocated while reading the options, as the commit message says ;-)
> > @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const
> > char *value, void *data)
>
> Sidenote: this patch would be easier to read if lines were reordered
> as below, but I don't think any slider heuristics could help achieve
> that automatically. Also, the patch might be invalid...
>
> > opts->allow_ff = git_config_bool_or_int(key, value,
> > &error_flag);
> > else if (!strcmp(key, "options.mainline"))
> > opts->mainline = git_config_int(key, value);
> > - else if (!strcmp(key, "options.strategy"))
> > + else if (!strcmp(key, "options.strategy")) {
> > git_config_string(&opts->strategy, key, value);
> > + sequencer_entrust(opts, (char *) opts->strategy);
>
> I wonder if the ability to free strings dup-ed by git_config_string()
> be something that is part of replay_opts, or rather remove_sequencer_state(),
> that is a list of
>
> free(opts->strategy);
> free(opts->gpg_sign);
That is not necessarily possible because the way sequencer works, the
options may have not actually be read from the file, but may be populated
by the caller (in which case we do not necessarily want to require
strdup()ing the strings just so that the sequencer can clean stuff up
afterwards).
> Though... free(NULL) is nop as per standard, but can we rely on it?
We can, and we do.
> The *_entrust() mechanism is more generic, but do we use this general-ness?
> Well, it could be xstrdup or git_config_string doing entrust'ing...
Right, but that is exactly what I wanted to avoid, because it is rather
inelegant to strdup() strings just so that we do not have to record what
to free() and what not to free().
BTW I have no objection at all to generalize this sequencer_entrust()
mechanism further (read: to other, similar use cases), should it withstand
the test of time.
Ciao,
Johannes
Re: [PATCH 06/22] sequencer: release memory that was allocated when reading options
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> The sequencer reads options from disk and stores them in its struct
> for use during sequencer's operations.
>
> With this patch, the memory is released afterwards, plugging a
> memory leak.
>
> Signed-off-by: Johannes Schindelin
> ---
> sequencer.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b5be0f9..8d79091 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct
> replay_opts *opts)
> free(opts->owned[i]);
> free(opts->owned);
>
> + free(opts->xopts);
> +
This looks like independent change, not related to using the
sequencer_entrust() to store options read from disk in replay_opts
struct to be able to free memory afterwards.
I guess you wanted to avoid one line changes...
> strbuf_addf(&dir, "%s", get_dir(opts));
> remove_dir_recursively(&dir, 0);
> strbuf_release(&dir);
> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char
> *value, void *data)
Sidenote: this patch would be easier to read if lines were reordered
as below, but I don't think any slider heuristics could help achieve
that automatically. Also, the patch might be invalid...
> opts->allow_ff = git_config_bool_or_int(key, value,
> &error_flag);
> else if (!strcmp(key, "options.mainline"))
> opts->mainline = git_config_int(key, value);
> - else if (!strcmp(key, "options.strategy"))
> + else if (!strcmp(key, "options.strategy")) {
> git_config_string(&opts->strategy, key, value);
> + sequencer_entrust(opts, (char *) opts->strategy);
I wonder if the ability to free strings dup-ed by git_config_string()
be something that is part of replay_opts, or rather remove_sequencer_state(),
that is a list of
free(opts->strategy);
free(opts->gpg_sign);
And of course
for (i = 0; i < opts->xopts_nr; i++)
free(opts->xopts[i]);
free(opts->xopts);
Though... free(NULL) is nop as per standard, but can we rely on it?
If it is a problem, we can create xfree(ptr) being if(ptr)free(ptr);
The *_entrust() mechanism is more generic, but do we use this general-ness?
Well, it could be xstrdup or git_config_string doing entrust'ing...
> + }
> - else if (!strcmp(key, "options.gpg-sign"))
> + else if (!strcmp(key, "options.gpg-sign")) {
> git_config_string(&opts->gpg_sign, key, value);
> + sequencer_entrust(opts, (char *) opts->gpg_sign);
> + }
> else if (!strcmp(key, "options.strategy-option")) {
> ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> - opts->xopts[opts->xopts_nr++] = xstrdup(value);
> + opts->xopts[opts->xopts_nr++] =
> + sequencer_entrust(opts, xstrdup(value));
Nice.
> } else
> return error(_("Invalid key: %s"), key);
>
>
[PATCH 06/22] sequencer: release memory that was allocated when reading options
The sequencer reads options from disk and stores them in its struct
for use during sequencer's operations.
With this patch, the memory is released afterwards, plugging a
memory leak.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index b5be0f9..8d79091 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct replay_opts
*opts)
free(opts->owned[i]);
free(opts->owned);
+ free(opts->xopts);
+
strbuf_addf(&dir, "%s", get_dir(opts));
remove_dir_recursively(&dir, 0);
strbuf_release(&dir);
@@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char
*value, void *data)
opts->allow_ff = git_config_bool_or_int(key, value,
&error_flag);
else if (!strcmp(key, "options.mainline"))
opts->mainline = git_config_int(key, value);
- else if (!strcmp(key, "options.strategy"))
+ else if (!strcmp(key, "options.strategy")) {
git_config_string(&opts->strategy, key, value);
- else if (!strcmp(key, "options.gpg-sign"))
+ sequencer_entrust(opts, (char *) opts->strategy);
+ }
+ else if (!strcmp(key, "options.gpg-sign")) {
git_config_string(&opts->gpg_sign, key, value);
+ sequencer_entrust(opts, (char *) opts->gpg_sign);
+ }
else if (!strcmp(key, "options.strategy-option")) {
ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
- opts->xopts[opts->xopts_nr++] = xstrdup(value);
+ opts->xopts[opts->xopts_nr++] =
+ sequencer_entrust(opts, xstrdup(value));
} else
return error(_("Invalid key: %s"), key);
--
2.10.0.rc1.114.g2bd6b38
