Re: [PATCH v2 25/25] refs: document the lifetime of the args passed to each_ref_fn
Michael Haggerty writes:
> The commits leading up to this have (hopefully) fixed all of the
> callers of the for_each_ref()-like functions. This commit does the
> last step: documents what each_ref_fn callbacks can assume about
> object lifetimes.
>
> Signed-off-by: Michael Haggerty
I looked at all the patches in the series and they all looked
sensible.
I have a few comments (sent as individual review) but all of the
suggestions in them are "by the way I noticed this issue that is not
new to this series while I was reading the code, and we may want to
fix it elsewhere", not "this is broken in the patch, we need to fix
it before queuing".
Thanks for a pleasant read.
> ---
> refs.h | 22 --
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/refs.h b/refs.h
> index a35eafc..122ec03 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -15,13 +15,23 @@ struct ref_lock {
> #define REF_ISBROKEN 0x04
>
> /*
> - * Calls the specified function for each ref file until it returns
> - * nonzero, and returns the value. Please note that it is not safe to
> - * modify references while an iteration is in progress, unless the
> - * same callback function invocation that modifies the reference also
> - * returns a nonzero value to immediately stop the iteration.
> + * The signature for the callback function for the for_each_*()
> + * functions below. The memory pointed to by the refname and sha1
> + * arguments is only guaranteed to be valid for the duration of a
> + * single callback invocation.
> + */
> +typedef int each_ref_fn(const char *refname,
> + const unsigned char *sha1, int flags, void *cb_data);
> +
> +/*
> + * The following functions invoke the specified callback function for
> + * each reference indicated. If the function ever returns a nonzero
> + * value, stop the iteration and return that value. Please note that
> + * it is not safe to modify references while an iteration is in
> + * progress, unless the same callback function invocation that
> + * modifies the reference also returns a nonzero value to immediately
> + * stop the iteration.
> */
> -typedef int each_ref_fn(const char *refname, const unsigned char *sha1, int
> flags, void *cb_data);
> extern int head_ref(each_ref_fn, void *);
> extern int for_each_ref(each_ref_fn, void *);
> extern int for_each_ref_in(const char *, each_ref_fn, void *);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 25/25] refs: document the lifetime of the args passed to each_ref_fn
The lifetime of the memory pointed to by the refname and sha1
arguments to each_ref_fn was never documented, but some callers used
to assume that it was essentially permanent. In fact the API does
*not* guarantee that these objects live beyond a single callback
invocation.
In the current code, the lifetimes are bound together with the
lifetimes of the ref_caches. Since these are usually long, the
callers usually got away with their sloppiness. But even today, if a
ref_cache is invalidated the memory can be freed. And planned changes
to reference caching, needed to eliminate race conditions, will
probably need to shorten the lifetimes of these objects.
The commits leading up to this have (hopefully) fixed all of the
callers of the for_each_ref()-like functions. This commit does the
last step: documents what each_ref_fn callbacks can assume about
object lifetimes.
Signed-off-by: Michael Haggerty
---
refs.h | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/refs.h b/refs.h
index a35eafc..122ec03 100644
--- a/refs.h
+++ b/refs.h
@@ -15,13 +15,23 @@ struct ref_lock {
#define REF_ISBROKEN 0x04
/*
- * Calls the specified function for each ref file until it returns
- * nonzero, and returns the value. Please note that it is not safe to
- * modify references while an iteration is in progress, unless the
- * same callback function invocation that modifies the reference also
- * returns a nonzero value to immediately stop the iteration.
+ * The signature for the callback function for the for_each_*()
+ * functions below. The memory pointed to by the refname and sha1
+ * arguments is only guaranteed to be valid for the duration of a
+ * single callback invocation.
+ */
+typedef int each_ref_fn(const char *refname,
+ const unsigned char *sha1, int flags, void *cb_data);
+
+/*
+ * The following functions invoke the specified callback function for
+ * each reference indicated. If the function ever returns a nonzero
+ * value, stop the iteration and return that value. Please note that
+ * it is not safe to modify references while an iteration is in
+ * progress, unless the same callback function invocation that
+ * modifies the reference also returns a nonzero value to immediately
+ * stop the iteration.
*/
-typedef int each_ref_fn(const char *refname, const unsigned char *sha1, int
flags, void *cb_data);
extern int head_ref(each_ref_fn, void *);
extern int for_each_ref(each_ref_fn, void *);
extern int for_each_ref_in(const char *, each_ref_fn, void *);
--
1.8.2.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
