Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile
On Mon, Aug 25, 2014 at 10:16:52AM -0700, Ronnie Sahlberg wrote: > Print an error before returning when pack_data is NULL ? I don't think so. We call end_packfile in some code paths (like the die handler) as "close if it's open". So I think it makes sense for it to be a noop if nothing is open. -Peff -- 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
Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile
Print an error before returning when pack_data is NULL ?
Otherwise, LGTM
On Fri, Aug 22, 2014 at 10:27 PM, Jeff King wrote:
> We have a global pointer pack_data pointing to the current
> pack we have open. Inside end_packfile we have two new
> pointers, old_p and new_p. The latter points to pack_data,
> and the former points to the new "installed" version of the
> packfile we get when we hand the file off to the regular
> sha1_file machinery. When then free old_p.
>
> Presumably the extra old_p pointer was there so that we
> could overwrite pack_data with new_p and still free old_p,
> but we don't do that. We just leave pack_data pointing to
> bogus memory, and don't overwrite it until we call
> start_packfile again (if ever).
>
> This can cause problems for our die routine, which calls
> end_packfile to clean things up. If we die at the wrong
> moment, we can end up looking at invalid memory in
> pack_data left after the last end_packfile().
>
> Instead, let's make sure we set pack_data to NULL after we
> free it, and make calling endfile() again with a NULL
> pack_data a noop (there is nothing to end).
>
> We can further make things less confusing by dropping old_p
> entirely, and moving new_p closer to its point of use.
>
> Signed-off-by: Jeff King
> ---
> Noticed while running fast-import under valgrind to debug the next
> commit. :)
>
> fast-import.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index d73f58c..f25a4ae 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -946,10 +946,12 @@ static void unkeep_all_packs(void)
>
> static void end_packfile(void)
> {
> - struct packed_git *old_p = pack_data, *new_p;
> + if (!pack_data)
> + return;
>
> clear_delta_base_cache();
> if (object_count) {
> + struct packed_git *new_p;
> unsigned char cur_pack_sha1[20];
> char *idx_name;
> int i;
> @@ -991,10 +993,11 @@ static void end_packfile(void)
> pack_id++;
> }
> else {
> - close(old_p->pack_fd);
> - unlink_or_warn(old_p->pack_name);
> + close(pack_data->pack_fd);
> + unlink_or_warn(pack_data->pack_name);
> }
> - free(old_p);
> + free(pack_data);
> + pack_data = NULL;
>
> /* We can't carry a delta across packfiles. */
> strbuf_release(&last_blob.data);
> --
> 2.1.0.346.ga0367b9
>
--
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 3/5] fast-import: clean up pack_data pointer in end_packfile
We have a global pointer pack_data pointing to the current
pack we have open. Inside end_packfile we have two new
pointers, old_p and new_p. The latter points to pack_data,
and the former points to the new "installed" version of the
packfile we get when we hand the file off to the regular
sha1_file machinery. When then free old_p.
Presumably the extra old_p pointer was there so that we
could overwrite pack_data with new_p and still free old_p,
but we don't do that. We just leave pack_data pointing to
bogus memory, and don't overwrite it until we call
start_packfile again (if ever).
This can cause problems for our die routine, which calls
end_packfile to clean things up. If we die at the wrong
moment, we can end up looking at invalid memory in
pack_data left after the last end_packfile().
Instead, let's make sure we set pack_data to NULL after we
free it, and make calling endfile() again with a NULL
pack_data a noop (there is nothing to end).
We can further make things less confusing by dropping old_p
entirely, and moving new_p closer to its point of use.
Signed-off-by: Jeff King
---
Noticed while running fast-import under valgrind to debug the next
commit. :)
fast-import.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index d73f58c..f25a4ae 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -946,10 +946,12 @@ static void unkeep_all_packs(void)
static void end_packfile(void)
{
- struct packed_git *old_p = pack_data, *new_p;
+ if (!pack_data)
+ return;
clear_delta_base_cache();
if (object_count) {
+ struct packed_git *new_p;
unsigned char cur_pack_sha1[20];
char *idx_name;
int i;
@@ -991,10 +993,11 @@ static void end_packfile(void)
pack_id++;
}
else {
- close(old_p->pack_fd);
- unlink_or_warn(old_p->pack_name);
+ close(pack_data->pack_fd);
+ unlink_or_warn(pack_data->pack_name);
}
- free(old_p);
+ free(pack_data);
+ pack_data = NULL;
/* We can't carry a delta across packfiles. */
strbuf_release(&last_blob.data);
--
2.1.0.346.ga0367b9
--
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
