Jared Hance <jaredhance@xxxxxxxxx> writes:
> In addition, the list of fragments should be free'd. To fix this, the
> utility function free_patch has been implemented. It loops over the
> entire patch list, and in each patch, loops over the fragment list,
> freeing the fragments, followed by the patch in the list. It frees both
> patch and patch->next.
Right encapsulation and abstraction. Good.
>
> The main caveat is that the text in a fragment, ie,
> patch->fragments->patch, may or may not need to be free'd. The text is
> dynamically allocated and needs to be freed iff the patch is a binary
> patch, as allocation occurs in inflate_it.
Can't we do better than "is this for a binary patch"? I find this part
not very forward-thinking implementation that relies on an implementation
detail that happens to hold true for today's code.
At least
if ((buf.buf <= fragment->patch &&
(fragment->patch < buf.buf + buf.len))
; /* this is inside the original buffer */
else
free(fragment->patch);
or something?
Strictly speaking, even the above is not forward-thinking enough, as the
code deep in the callchain is free to replace ->patch with an unfreeable
string. I think the right way to handle this is to add a single bitfield
"should_free" to the struct fragment and default it to 'false', and make
the place that replace the patch field with different string responsible
for flipping the bit to 'true'. Your "free_patch()" can then rely on that
bit to make the decision.
> Signed-off-by: Jared Hance <jaredhance@xxxxxxxxx>
> ---
> builtin/apply.c | 30 +++++++++++++++++++++++++++---
> 1 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 389898f..a73d339 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -196,6 +196,30 @@ struct patch {
> struct patch *next;
> };
>
> +static void free_patch(struct patch *patch) {
> + while(patch != NULL) {
Style:
static void free_patch(struct patch *patch)
{
while (patch) {
...
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html