Re: [PATCH 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Christian Couder]

On Mon, Apr 25, 2016 at 9:50 AM, Eric Sunshine  wrote:
>> @@ -4515,8 +4521,6 @@ static int write_out_results(struct apply_state 
>> *state, struct patch *list)
>> return errs;
>>  }
>>
>> -static struct lock_file lock_file;
>
> Does the static lock_file in build_fake_ancestor() deserve the same
> sort of treatment? (I haven't traced the code enough to answer this.)

Maybe yes we could do the same thing for this static lock_file, but
this can be done later, and it could be a bit involved, so I prefer to
not touch that for now.

We are using the lock_file like this in build_fake_ancestor():

hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
if (write_locked_index(, , COMMIT_LOCK))
return error("Could not write temporary index to %s", filename);

so it looks like it is safe to call build_fake_ancestor() many times
as long as it is not called by different threads.
--
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 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Junio C Hamano]

Christian Couder  writes:

>> I do not think you need to think about "free"ing.
>
> Yeah, lockfile.h says:
> ...
> and:
> ...

Yup, we are now on the same page.

>> Even if the libified version of the apply internal can be called
>> multiple times to process multiple patch inputs, there is no need to
>> run multiple instances of it yet.  And a lockfile, after the caller
>> finishes interacting with one file using it by calling commit or
>> rollback, can be reused to interact with another file.

lockfile.h says this about the above paragraph, which is a more
important part ;-)

 * When finished writing, the caller can:
 * ...
 * Even after the lockfile is committed or rolled back, the
 * `lock_file` object must not be freed or altered by the caller.
 * However, it may be reused; just pass it to another call of
 * `hold_lock_file_for_update()`.

--
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 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Christian Couder]

On Mon, Apr 25, 2016 at 7:55 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>
>>> +   /*
>>> +* 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 there ever a time when lock_file is removed from the list (such as
>> upon successful write of the index), in which case it would be safe to
>> free() this?
>
> I do not think you need to think about "free"ing.

Yeah, lockfile.h says:

 * * Automatic cruft removal. If the program exits after we lock a
 *   file but before the changes have been committed, we want to make
 *   sure that we remove the lockfile. This is done by remembering the
 *   lockfiles we have created in a linked list and setting up an
 *   `atexit(3)` handler and a signal handler that clean up the
 *   lockfiles. This mechanism ensures that outstanding lockfiles are
 *   cleaned up if the program exits (including when `die()` is
 *   called) or if the program is terminated by a signal.

and:

 * The caller:
 *
 * * Allocates a `struct lock_file` either as a static variable or on
 *   the heap, initialized to zeros. Once you use the structure to
 *   call the `hold_lock_file_for_*()` family of functions, it belongs
 *   to the lockfile subsystem and its storage must remain valid
 *   throughout the life of the program (i.e. you cannot use an
 *   on-stack variable to hold this structure).

> Even if the libified version of the apply internal can be called
> multiple times to process multiple patch inputs, there is no need to
> run multiple instances of it yet.  And a lockfile, after the caller
> finishes interacting with one file using it by calling commit or
> rollback, can be reused to interact with another file.
>
> So the cleanest would be to:
>
>  * make the caller of apply API responsible for preparing a static
>or (allocating and leaking) lockfile instance,

Ok.

>  * make it pass a pointer to the lockfile to the apply API so that
>it can store it in apply_state, and

Ok, I will add a new "struct lock_file *" parameter to
init_apply_state(), for the caller of the apply API to do that.
Though I think it should be ok for the caller to pass a NULL pointer
and in this case have init_apply_state() allocate the lockfile file
instance.

>  * have the caller use apply API feeding one patch at a time in a
>loop, allowing the same lockfile instance used for each
>"invocation".

Ok, the same lockfile instance will be used for each invocation.

> I sounded as if I were advocating non-reentrant implementation in
> the introductory paragraph, but that is not the intention.  If you
> want to do N threads, you would prepare N lockfile instances, give
> them to N apply API instances to be stored in N apply_state
> instances, and you would have N parallel applications, if you wanted
> to.

Thanks,
Christian.
--
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 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Junio C Hamano]

Eric Sunshine  writes:

>> +   /*
>> +* 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 there ever a time when lock_file is removed from the list (such as
> upon successful write of the index), in which case it would be safe to
> free() this?

I do not think you need to think about "free"ing.

Even if the libified version of the apply internal can be called
multiple times to process multiple patch inputs, there is no need to
run multiple instances of it yet.  And a lockfile, after the caller
finishes interacting with one file using it by calling commit or
rollback, can be reused to interact with another file.

So the cleanest would be to:

 * make the caller of apply API responsible for preparing a static
   or (allocating and leaking) lockfile instance,

 * make it pass a pointer to the lockfile to the apply API so that
   it can store it in apply_state, and

 * have the caller use apply API feeding one patch at a time in a
   loop, allowing the same lockfile instance used for each
   "invocation".

I sounded as if I were advocating non-reentrant implementation in
the introductory paragraph, but that is not the intention.  If you
want to do N threads, you would prepare N lockfile instances, give
them to N apply API instances to be stored in N apply_state
instances, and you would have N parallel applications, if you wanted
to.
--
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 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Eric Sunshine]

On Sun, Apr 24, 2016 at 9:33 AM, Christian Couder
 wrote:
> We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
> keeps a linked list of all created lock_file structures.

By talking about "the stack" here, I suppose you mean that your
initial idea was to move the global lock_file into cmd_apply() or
something?

> So let's make the 'lock_file' variable a pointer to a 'struct lock_file'
> and let's alloc the struct when needed.
>
> Signed-off-by: Christian Couder 
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -52,6 +52,12 @@ struct apply_state {
> +   /*
> +* 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 there ever a time when lock_file is removed from the list (such as
upon successful write of the index), in which case it would be safe to
free() this?

> @@ -4515,8 +4521,6 @@ static int write_out_results(struct apply_state *state, 
> struct patch *list)
> return errs;
>  }
>
> -static struct lock_file lock_file;

Does the static lock_file in build_fake_ancestor() deserve the same
sort of treatment? (I haven't traced the code enough to answer this.)

>  #define INACCURATE_EOF (1<<0)
>  #define RECOUNT(1<<1)
--
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 49/83] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Christian Couder]

We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
keeps a linked list of all created lock_file structures.
So let's make the 'lock_file' variable a pointer to a 'struct lock_file'
and let's alloc the struct when needed.

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 6c0b153..d26419a 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;
+
int apply;
int allow_overlap;
int apply_in_reverse;
@@ -4515,8 +4521,6 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
return errs;
 }
 
-static struct lock_file lock_file;
-
 #define INACCURATE_EOF (1<<0)
 #define RECOUNT(1<<1)
 
@@ -4568,8 +4572,10 @@ static int apply_patch(struct apply_state *state,
state->apply = 0;
 
state->update_index = state->check_index && state->apply;
-   if (state->update_index && newfd < 0)
-   newfd = hold_locked_index(_file, 1);
+   if (state->update_index && newfd < 0) {
+   state->lock_file = xcalloc(1, sizeof(struct lock_file));
+   newfd = hold_locked_index(state->lock_file, 1);
+   }
 
if (state->check_index) {
if (read_cache() < 0)
@@ -4782,7 +4788,7 @@ static int apply_all_patches(struct apply_state *state,
}
 
if (state->update_index) {
-   if (write_locked_index(_index, _file, COMMIT_LOCK))
+   if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
die(_("Unable to write new index file"));
}
 
-- 
2.8.1.300.g5fed0c0

--
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