Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Christian Couder <christian.couder@xxxxxxxxx> writes:

> This is to replace:
>
> "[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'"
>
> from the "libify apply and use lib in am, part 1" patch series.

Thanks; will replace the tip 2 patches and requeue.

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 5027f1b..cc635eb 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -52,6 +52,12 @@ struct apply_state {
>  	const char *prefix;
>  	int prefix_length;
>  
> +	/*
> +	 * Since lockfile.c keeps a linked list of all created
> +	 * lock_file structures, it isn't safe to free(lock_file).
> +	 */
> +	struct lock_file *lock_file;

Is this even an issue for this thing anymore?  It is the
responsibilty of the API user to manage the lifetime of what
lock_file points at.  The caller may have it on heap and let it
leak, or it may have in BSS in which case it won't even dream about
freeing it.

If a comment were needed for this field, it should say "when
discarding/freeing apply_state, just discard this pointer without
touching what the pointer points at; the storage the pointer points
at does not belong to the API implementation but belongs to the API
user".

But I do not think such a comment is needed, because the situation
is the same as other fields like *prefix, and for the same reason we
do not do anything to these fields in clear_apply_state().

Other than that this looks great.
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]