Notifications support #52

2023-06-08T12:51:26Z

GiacomoCortesi commented

2023-06-08 12:51:26 +00:00

(Migrated from github.com)

implementation of netconf notifications as per proposal in https://github.com/nemith/netconf/issues/51

I've added a couple of TODOs that we may or may not want to address.

Filter parameter implementation may be out of scope and deserve its own PR, what plans have you for it?

About checks on capabilities I'm not sure you actually want to address them here, at least I don't see other places where you take capabilities into account.

implementation of netconf notifications as per proposal in https://github.com/nemith/netconf/issues/51 I've added a couple of TODOs that we may or may not want to address. Filter parameter implementation may be out of scope and deserve its own PR, what plans have you for it? About checks on capabilities I'm not sure you actually want to address them here, at least I don't see other places where you take capabilities into account.

nemith (Migrated from github.com) requested changes

2023-06-08 13:00:41 +00:00

nemith (Migrated from github.com) left a comment

Looks good.

Couple minor nits and it seems like you may need to rebase on top of master (there was some changes with how recv() works.

Looks good. Couple minor nits and it seems like you may need to rebase on top of master (there was some changes with how recv() works.

ops.go

					
				@ -480,3 +480,46 @@ func (s *Session) CancelCommit(ctx context.Context, opts ...CancelCommitOption)

					var resp OKResp

Being that this is static i think it's best to just put the namespace into XMLName and not worry about copying it. I fought this same thing with the base netconf namespacing as well.

ops.go

					
				@ -483,0 +518,4 @@

					for _, opt := range opts {

						opt.apply(&req)

					}

					// TODO: eventual custom notifications rpc logic, e.g. create subscription only if notification capability is present

I fought this idea too. What should we do if capability is missing, error out? I think it's best left for the future but it is an interesting ida.

session.go

The beset practice with mutex locks is to put under them what they are locking. So notificationHandler being here implies it's locked by the mutex lock (and it isn't)

Maybe move it up around the capbilities instead?

The beset practice with mutex locks is to put under them what they are locking. So notificationHandler being here implies it's locked by the mutex lock (and it isn't) Maybe move it up around the capbilities instead?

session.go

notifNamespace is probably a slightly better name. I was a bit confused when i say notNamespace

`notifNamespace` is probably a slightly better name. I was a bit confused when i say `notNamespace`

session.go

					
				@ -176,16 +202,21 @@ func (s *Session) recvMsg() error {

						return err

					}

This nil check should be done before we decode the notification. This if no handler is present we just drop the message instead of spending time decoding it.

the reader.Close() should advance the reader to the end of the message.

This nil check should be done before we decode the notification. This if no handler is present we just drop the message instead of spending time decoding it. the reader.Close() should advance the reader to the end of the message.

GiacomoCortesi (Migrated from github.com) reviewed

2023-06-08 14:15:15 +00:00

ops.go

					
				@ -483,0 +518,4 @@

					for _, opt := range opts {

						opt.apply(&req)

					}

					// TODO: eventual custom notifications rpc logic, e.g. create subscription only if notification capability is present

I agree on leaving it to the future, need some deeper thinking

GiacomoCortesi commented

2023-06-08 14:27:23 +00:00

(Migrated from github.com)

Looks good.

Couple minor nits and it seems like you may need to rebase on top of master (there was some changes with how recv() works.

I rebased and applied fixes as per comments

> Looks good. > > Couple minor nits and it seems like you may need to rebase on top of master (there was some changes with how recv() works. I rebased and applied fixes as per comments

nemith (Migrated from github.com) approved these changes

2023-06-08 15:51:24 +00:00

nemith (Migrated from github.com) left a comment

LGTM to me!

session.go

					
				@ -176,16 +202,21 @@ func (s *Session) recvMsg() error {

						return err

					}

minor nit make this a const block

	const (
		ncNamespace = "urn:ietf:params:xml:ns:netconf:base:1.0"
		notifNamespace = "urn:ietf:params:xml:ns:netconf:notification:1.0"
	)

minor nit make this a const block ```suggestion const ( ncNamespace = "urn:ietf:params:xml:ns:netconf:base:1.0" notifNamespace = "urn:ietf:params:xml:ns:netconf:notification:1.0" ) ```

Sign in to join this conversation.

No reviewers

nemith