Re: [PATCH samba v3 1/1] smbclient: cancel pending notify on SIGINT

Hi Ralph,

Do you have any suggestions for this patch?

I also created a merge request on GitLab: 
https://gitlab.com/samba-team/samba/-/merge_requests/4530

On 6/8/26 22:28, [email protected] wrote:
> From: ChenXiaoSong <[email protected]>
> 
> When client stops querying a directory for change notifications,
> it should send a cancel request to the server.
> 
> After cancel notify succeeds, `status` returned by both
> `tevent_req_poll_ntstatus()` and `cli_notify_recv()` is
> `NT_STATUS_OK` rather than `NT_STATUS_CANCELLED`, so we
> can only check `sig_state.cancelled`.
> 
> Suggested-by: Stefan Metzmacher <[email protected]>
> Signed-off-by: ChenXiaoSong <[email protected]>
> ---
>   source3/client/client.c | 64 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 57 insertions(+), 7 deletions(-)
> 
> diff --git a/source3/client/client.c b/source3/client/client.c
> index 1d3656aab34..37693b8f401 100644
> --- a/source3/client/client.c
> +++ b/source3/client/client.c
> @@ -45,6 +45,7 @@
>   #include "lib/cmdline/cmdline.h"
>   #include "libcli/smb/reparse.h"
>   #include "lib/param/param.h"
> +#include "lib/util/tevent_ntstatus.h"
>   
>   #ifndef REGISTER
>   #define REGISTER 0
> @@ -4653,19 +4654,46 @@ static int cmd_newer(TALLOC_CTX *mem_ctx)
>    Watch directory changes
>   ****************************************************************************/
>   
> +struct notify_state {
> +	struct tevent_req *notify_req;
> +	bool cancelled;
> +};
> +
> +static void
> +notify_sig_int_handler(
> +	struct tevent_context *ev,
> +	struct tevent_signal *se,
> +	int signum,
> +	int count,
> +	void *siginfo,
> +	void *private_data)
> +{
> +	struct notify_state *state = (struct notify_state *)private_data;
> +
> +	if (state->notify_req)
> +		state->cancelled = tevent_req_cancel(state->notify_req);
> +}
> +
>   static int cmd_notify(TALLOC_CTX *mem_ctx)
>   {
>   	TALLOC_CTX *frame = talloc_stackframe();
>   	char *name, *buf;
>   	NTSTATUS status;
>   	uint16_t fnum;
> +	struct tevent_req *req;
> +	struct tevent_context *ev;
> +	struct tevent_signal *se;
> +	struct notify_state sig_state = {
> +		.notify_req = NULL,
> +		.cancelled = false,
> +	};
>   
>   	name = talloc_strdup(talloc_tos(), client_get_cur_dir());
>   	if (name == NULL) {
>   		goto fail;
>   	}
>   	if (!next_token_talloc(talloc_tos(), &cmd_ptr, &buf, NULL)) {
> -		goto usage;
> +		goto fail;
>   	}
>   	name = talloc_asprintf_append(name, "%s", buf);
>   	if (name == NULL) {
> @@ -4684,14 +4712,33 @@ static int cmd_notify(TALLOC_CTX *mem_ctx)
>   		goto fail;
>   	}
>   
> +	ev = samba_tevent_context_init(frame);
> +	if (!ev)
> +		goto close;
> +
> +	se = tevent_add_signal(ev, frame, SIGINT, 0, notify_sig_int_handler, &sig_state);
> +	if (!se)
> +		goto close;
> +
>   	while (1) {
>   		uint32_t i;
>   		uint32_t num_changes = 0;
>   		struct notify_change *changes = NULL;
>   
> -		status = cli_notify(cli, fnum, 1000, FILE_NOTIFY_CHANGE_ALL,
> -				    true,
> -				    talloc_tos(), &num_changes, &changes);
> +		if (sig_state.cancelled) {
> +			printf("notify cancelled\n");
> +			goto close;
> +		}
> +
> +		req = cli_notify_send(frame, ev, cli, fnum, 1000, FILE_NOTIFY_CHANGE_ALL, true);
> +		if (!req)
> +			goto close;
> +		sig_state.notify_req = req;
> +
> +		if (!tevent_req_poll_ntstatus(req, ev, &status))
> +			goto close;
> +
> +		status = cli_notify_recv(req, talloc_tos(), &num_changes, &changes);
>   		if (NT_STATUS_EQUAL(status, NT_STATUS_NOTIFY_ENUM_DIR)) {
>   			printf("NOTIFY_ENUM_DIR\n");
>   			status = NT_STATUS_OK;
> @@ -4699,7 +4746,7 @@ static int cmd_notify(TALLOC_CTX *mem_ctx)
>   		if (!NT_STATUS_IS_OK(status)) {
>   			d_printf("notify returned %s\n",
>   				 nt_errstr(status));
> -			goto fail;
> +			goto close;
>   		}
>   		for (i=0; i<num_changes; i++) {
>   			printf("%4.4x %s\n", changes[i].action,
> @@ -4707,8 +4754,11 @@ static int cmd_notify(TALLOC_CTX *mem_ctx)
>   		}
>   		TALLOC_FREE(changes);
>   	}
> -usage:
> -	d_printf("notify <dir name>\n");
> +close:
> +	status = cli_close(cli, fnum);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		d_printf("close returned %s\n", nt_errstr(status));
> +	}
>   fail:
>   	TALLOC_FREE(frame);
>   	return 1;

-- 
ChenXiaoSong <[email protected]>
Chinese Homepage: https://chenxiaosong.com
English Homepage: https://chenxiaosong.com/en