Re: [PATCH v3 03/10] pkt-line: add packet_flush_gentle()

[Lars Schneider]

> On 02 Aug 2016, at 21:56, Torsten Bögershausen  wrote:
> 
> On Sun, Jul 31, 2016 at 11:45:08PM +0200, Lars Schneider wrote:
>> 
>>> On 31 Jul 2016, at 22:36, Torstem Bögershausen  wrote:
>>> 
>>> 
>>> 
 Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com:
 
 From: Lars Schneider 
 
 packet_flush() would die in case of a write error even though for some 
 callers
 an error would be acceptable.
>>> What happens if there is a write error ?
>>> Basically the protocol is out of synch.
>>> Lenght information is mixed up with payload, or the other way
>>> around.
>>> It may be, that the consequences of a write error are acceptable,
>>> because a filter is allowed to fail.
>>> What is not acceptable is a "broken" protocol.
>>> The consequence schould be to close the fd and tear down all
>>> resources. connected to it.
>>> In our case to terminate the external filter daemon in some way,
>>> and to never use this instance again.
>> 
>> Correct! That is exactly what is happening in kill_protocol2_filter()
>> here:
> 
> Wait a second.
> Is kill the same as shutdown ?
> I would expect that

No, kill is used if the filter behaved strangely or signaled an error.
"Shutdown" is a graceful shutdown. However, that might not be an ideal
name. See the bottom of my discussion with Peff here:
http://public-inbox.org/git/74C2CEA6-EAAB-406F-8B37-969654955413%40gmail.com/


> The process terminates itself as soon as it detects EOF.
> As there is nothing more read.
> 
> Then the next question: The combination of kill & protocol in kill_protocol(),
> what does it mean ?

I renamed that function to "kill_multi_file_filter". Initially I called
the multi file filter "protocol" (bad decision I know) and named the
functions accordingly.


> Is it more like a graceful shutdown_protocol() ?

Yes.

Thanks,
Lars--
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 v3 03/10] pkt-line: add packet_flush_gentle()

[Torsten Bögershausen]

On Sun, Jul 31, 2016 at 11:45:08PM +0200, Lars Schneider wrote:
> 
> > On 31 Jul 2016, at 22:36, Torstem Bögershausen  wrote:
> > 
> > 
> > 
> >> Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com:
> >> 
> >> From: Lars Schneider 
> >> 
> >> packet_flush() would die in case of a write error even though for some 
> >> callers
> >> an error would be acceptable.
> > What happens if there is a write error ?
> > Basically the protocol is out of synch.
> > Lenght information is mixed up with payload, or the other way
> > around.
> > It may be, that the consequences of a write error are acceptable,
> > because a filter is allowed to fail.
> > What is not acceptable is a "broken" protocol.
> > The consequence schould be to close the fd and tear down all
> > resources. connected to it.
> > In our case to terminate the external filter daemon in some way,
> > and to never use this instance again.
> 
> Correct! That is exactly what is happening in kill_protocol2_filter()
> here:

Wait a second.
Is kill the same as shutdown ?
I would expect that
The process terminates itself as soon as it detects EOF.
As there is nothing more read.

Then the next question: The combination of kill & protocol in kill_protocol(),
what does it mean ?
Is it more like a graceful shutdown_protocol() ?

--
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 v3 03/10] pkt-line: add packet_flush_gentle()

[Lars Schneider]

> On 30 Jul 2016, at 14:04, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>> From: Lars Schneider 
>> 
>> packet_flush() would die in case of a write error even though for some 
>> callers
>> an error would be acceptable. Add packet_flush_gentle() which writes a 
>> pkt-line
>> flush packet and returns `0` for success and `1` for failure.
> 
> I think it should be packet_flush_gently(), as in "to flush gently",
> but this is only my opinion; I have not checked the naming rules and
> practices for the rest of Git codebase.

Agreed. This would match:

object.c:int type_from_string_gently(const char *str, ssize_t len, int gentle)

Thanks,
Lars

> 
>> 
>> Signed-off-by: Lars Schneider 
>> ---
> 

--
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 v3 03/10] pkt-line: add packet_flush_gentle()

[Lars Schneider]

> On 31 Jul 2016, at 22:36, Torstem Bögershausen  wrote:
> 
> 
> 
>> Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com:
>> 
>> From: Lars Schneider 
>> 
>> packet_flush() would die in case of a write error even though for some 
>> callers
>> an error would be acceptable.
> What happens if there is a write error ?
> Basically the protocol is out of synch.
> Lenght information is mixed up with payload, or the other way
> around.
> It may be, that the consequences of a write error are acceptable,
> because a filter is allowed to fail.
> What is not acceptable is a "broken" protocol.
> The consequence schould be to close the fd and tear down all
> resources. connected to it.
> In our case to terminate the external filter daemon in some way,
> and to never use this instance again.

Correct! That is exactly what is happening in kill_protocol2_filter()
here:


+static int apply_protocol2_filter(const char *path, const char *src, size_t 
len,
+   int fd, struct strbuf *dst, 
const char *cmd,
+   const int wanted_capability)
+{
...
+   if (ret) {
+   strbuf_swap(dst, );
+   } else {
+   if (!filter_result || strcmp(filter_result, "reject")) {
+   // Something went wrong with the protocol filter. Force 
shutdown!
+   error("external filter '%s' failed", cmd);
+   kill_protocol2_filter(_process_map, entry);
+   }
+   }
+   strbuf_release();
+   return ret;
+}

More context:
https://github.com/larsxschneider/git/blob/e128326070847ac596e8bb21adebc8abab2003fc/convert.c#L821

- Lars


> 
> 
>> Add packet_flush_gentle() which writes a pkt-line
>> flush packet and returns `0` for success and `1` for failure.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> pkt-line.c | 6 ++
>> pkt-line.h | 1 +
>> 2 files changed, 7 insertions(+)
>> 
>> diff --git a/pkt-line.c b/pkt-line.c
>> index 6fae508..1728690 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -91,6 +91,12 @@ void packet_flush(int fd)
>> write_or_die(fd, "", 4);
>> }
>> 
>> +int packet_flush_gentle(int fd)
>> +{
>> +packet_trace("", 4, 1);
>> +return !write_or_whine_pipe(fd, "", 4, "flush packet");
>> +}
>> +
>> void packet_buf_flush(struct strbuf *buf)
>> {
>> packet_trace("", 4, 1);
>> diff --git a/pkt-line.h b/pkt-line.h
>> index 02dcced..3953c98 100644
>> --- a/pkt-line.h
>> +++ b/pkt-line.h
>> @@ -23,6 +23,7 @@ void packet_flush(int fd);
>> void packet_write(int fd, const char *fmt, ...) __attribute__((format 
>> (printf, 2, 3)));
>> void packet_buf_flush(struct strbuf *buf);
>> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
>> __attribute__((format (printf, 2, 3)));
>> +int packet_flush_gentle(int fd);
>> int direct_packet_write(int fd, char *buf, size_t size, int gentle);
>> int direct_packet_write_data(int fd, const char *buf, size_t size, int 
>> gentle);
>> 
>> -- 
>> 2.9.0
>> 

--
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 v3 03/10] pkt-line: add packet_flush_gentle()

[Torstem Bögershausen]

> Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com:
> 
> From: Lars Schneider 
> 
> packet_flush() would die in case of a write error even though for some callers
> an error would be acceptable.
What happens if there is a write error ?
Basically the protocol is out of synch.
Lenght information is mixed up with payload, or the other way
around.
It may be, that the consequences of a write error are acceptable,
because a filter is allowed to fail.
What is not acceptable is a "broken" protocol.
The consequence schould be to close the fd and tear down all
resources. connected to it.
In our case to terminate the external filter daemon in some way,
and to never use this instance again.


> Add packet_flush_gentle() which writes a pkt-line
> flush packet and returns `0` for success and `1` for failure.
> 
> Signed-off-by: Lars Schneider 
> ---
> pkt-line.c | 6 ++
> pkt-line.h | 1 +
> 2 files changed, 7 insertions(+)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index 6fae508..1728690 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -91,6 +91,12 @@ void packet_flush(int fd)
>  write_or_die(fd, "", 4);
> }
> 
> +int packet_flush_gentle(int fd)
> +{
> +packet_trace("", 4, 1);
> +return !write_or_whine_pipe(fd, "", 4, "flush packet");
> +}
> +
> void packet_buf_flush(struct strbuf *buf)
> {
>  packet_trace("", 4, 1);
> diff --git a/pkt-line.h b/pkt-line.h
> index 02dcced..3953c98 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -23,6 +23,7 @@ void packet_flush(int fd);
> void packet_write(int fd, const char *fmt, ...) __attribute__((format 
> (printf, 2, 3)));
> void packet_buf_flush(struct strbuf *buf);
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
> +int packet_flush_gentle(int fd);
> int direct_packet_write(int fd, char *buf, size_t size, int gentle);
> int direct_packet_write_data(int fd, const char *buf, size_t size, int 
> gentle);
> 
> -- 
> 2.9.0
> 
--
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 v3 03/10] pkt-line: add packet_flush_gentle()

[Jakub Narębski]

W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
> From: Lars Schneider 
> 
> packet_flush() would die in case of a write error even though for some callers
> an error would be acceptable. Add packet_flush_gentle() which writes a 
> pkt-line
> flush packet and returns `0` for success and `1` for failure.

I think it should be packet_flush_gently(), as in "to flush gently",
but this is only my opinion; I have not checked the naming rules and
practices for the rest of Git codebase.

> 
> Signed-off-by: Lars Schneider 
> ---


--
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 v3 03/10] pkt-line: add packet_flush_gentle()

[larsxschneider]

From: Lars Schneider 

packet_flush() would die in case of a write error even though for some callers
an error would be acceptable. Add packet_flush_gentle() which writes a pkt-line
flush packet and returns `0` for success and `1` for failure.

Signed-off-by: Lars Schneider 
---
 pkt-line.c | 6 ++
 pkt-line.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index 6fae508..1728690 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -91,6 +91,12 @@ void packet_flush(int fd)
write_or_die(fd, "", 4);
 }
 
+int packet_flush_gentle(int fd)
+{
+   packet_trace("", 4, 1);
+   return !write_or_whine_pipe(fd, "", 4, "flush packet");
+}
+
 void packet_buf_flush(struct strbuf *buf)
 {
packet_trace("", 4, 1);
diff --git a/pkt-line.h b/pkt-line.h
index 02dcced..3953c98 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -23,6 +23,7 @@ void packet_flush(int fd);
 void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 
2, 3)));
 void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
+int packet_flush_gentle(int fd);
 int direct_packet_write(int fd, char *buf, size_t size, int gentle);
 int direct_packet_write_data(int fd, const char *buf, size_t size, int gentle);
 
-- 
2.9.0

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