From: Mark Jamsek <mark@jamsek.com>
Subject: Re: gotd flush packets
To: Omar Polo <op@omarpolo.com>, gameoftrees@openbsd.org
Date: Sat, 21 Jan 2023 19:47:42 +1100

On 23-01-20 08:40PM, Stefan Sperling wrote:
> On Fri, Jan 20, 2023 at 08:04:51PM +0100, Omar Polo wrote:
> > On 2023/01/20 19:57:59 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > >  - After sending our ref announcement we can receive a flush packet
> > >    if the client decides it does not want anything.
> > >  - The list of want lines during fetch is terminated by a flush packet.
> > >  - The list of ref-updates lines during send is terminated by a flush packet.
> > 
> > that's my understanding as well
> 
> The previous patch was wrong in several ways.
> The client can send multiple want/have lines, the patch only accepted one.
> The client can send a flush packet before 'done'.
> This broke 'git pull' from an older copy of the repo I keep around for testing.
> 
> This still prevents multiple flush packets in sequence, and still allows
> lots of other shenanigans. I don't really want to invest more time into
> fixing this before having some repeatable test cases, though.

Makes sense and looks good to me, ok

> diff refs/heads/main refs/heads/flushpkt
> commit - 7713cc5e4f5544e81909670d592e89526ed86c9b
> commit + 00f48d6b02f52795d1596995bb628a8fe45b9aa1
> blob - 0bc48fac834b679822e3314db38590b9da8f3575
> blob + c2bf744e16922aec4500424898af4499c5240308
> --- gotd/session.c
> +++ gotd/session.c
> @@ -79,6 +79,7 @@ static struct gotd_session_client {
>  	char				*packfile_path;
>  	char				*packidx_path;
>  	int				 nref_updates;
> +	int				 accept_flush_pkt;
>  } gotd_session_client;
>  
>  void gotd_session_sighdlr(int sig, short event, void *arg);
> @@ -1037,10 +1038,12 @@ session_dispatch_listener(int fd, short events, void *
>  				break;
>  			if (!client->is_writing) {
>  				client->state = GOTD_STATE_EXPECT_WANT;
> +				client->accept_flush_pkt = 1;
>  				log_debug("uid %d: expecting want-lines",
>  				    client->euid);
>  			} else if (client->is_writing) {
>  				client->state = GOTD_STATE_EXPECT_REF_UPDATE;
> +				client->accept_flush_pkt = 1;
>  				log_debug("uid %d: expecting ref-update-lines",
>  				    client->euid);
>  			} else
> @@ -1058,6 +1061,7 @@ session_dispatch_listener(int fd, short events, void *
>  			err = ensure_client_is_reading(client);
>  			if (err)
>  				break;
> +			client->accept_flush_pkt = 1;
>  			err = forward_want(client, &imsg);
>  			break;
>  		case GOTD_IMSG_REF_UPDATE:
> @@ -1077,6 +1081,7 @@ session_dispatch_listener(int fd, short events, void *
>  			if (err)
>  				break;
>  			client->state = GOTD_STATE_EXPECT_MORE_REF_UPDATES;
> +			client->accept_flush_pkt = 1;
>  			break;
>  		case GOTD_IMSG_HAVE:
>  			if (client->state != GOTD_STATE_EXPECT_HAVE) {
> @@ -1092,6 +1097,7 @@ session_dispatch_listener(int fd, short events, void *
>  			err = forward_have(client, &imsg);
>  			if (err)
>  				break;
> +			client->accept_flush_pkt = 1;
>  			break;
>  		case GOTD_IMSG_FLUSH:
>  			if (client->state == GOTD_STATE_EXPECT_WANT ||
> @@ -1109,6 +1115,19 @@ session_dispatch_listener(int fd, short events, void *
>  				    "unexpected flush-pkt received");
>  				break;
>  			}
> +			if (!client->accept_flush_pkt) {
> +				err = got_error_msg(GOT_ERR_BAD_REQUEST,
> +				    "unexpected flush-pkt received");
> +				break;
> +			}
> +
> +			/*
> +			 * Accept just one flush packet at a time.
> +			 * Future client state transitions will set this flag
> +			 * again if another flush packet is expected.
> +			 */
> +			client->accept_flush_pkt = 0;
> +
>  			log_debug("received flush-pkt from uid %d",
>  			    client->euid);
>  			if (client->state == GOTD_STATE_EXPECT_WANT) {
> @@ -1117,6 +1136,7 @@ session_dispatch_listener(int fd, short events, void *
>  				    client->euid);
>  			} else if (client->state == GOTD_STATE_EXPECT_HAVE) {
>  				client->state = GOTD_STATE_EXPECT_DONE;
> +				client->accept_flush_pkt = 1;
>  				log_debug("uid %d: expecting 'done'",
>  				    client->euid);
>  			} else if (client->state ==
> @@ -1144,6 +1164,7 @@ session_dispatch_listener(int fd, short events, void *
>  			if (err)
>  				break;
>  			client->state = GOTD_STATE_DONE;
> +			client->accept_flush_pkt = 1;
>  			err = send_packfile(client);
>  			break;
>  		default:
> @@ -1402,6 +1423,7 @@ session_main(const char *title, const char *repo_path,
>  	gotd_session_client.fd = -1;
>  	gotd_session_client.nref_updates = -1;
>  	gotd_session_client.delta_cache_fd = -1;
> +	gotd_session_client.accept_flush_pkt = 1;
>  
>  	imsg_init(&gotd_session.parent_iev.ibuf, GOTD_FILENO_MSG_PIPE);
>  	gotd_session.parent_iev.handler = session_dispatch;
>  
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68