[PATCH] notifyd

[Jeremy Allison]

Hi!
Attached find the latest incarnation of the notifyd I've
been talking about for a while now. Survived a few private
autobuilds.
Review appreciated!

Only issue I've found so far.

In source3/smbd/notifyd/notifyd.c

you have notifyd_snoop_broadcast()

1347 * We know the source but the log index does not match. This means we
1348 * lost a message. We just drop the whole peer and wait for the next
1349 * broadcast, which will then trigger a fresh database pull.
1350 */
.....
1386 for (i=0; i<state->num_peers; i++) {
1387 if (server_id_equal(&state->peers[i]->pid, &src)) {
1388
1389 DEBUG(10, ("%s: Applying changes to peer %u\n",
1390 __func__, (unsigned)i));
1391
1392 notifyd_apply_reclog(state->peers[i],
1393 msg + MESSAGE_HDR_LENGTH,
1394 msglen - MESSAGE_HDR_LENGTH);
1395
1396 state->peers[i]->last_broadcast = time(NULL);
1397 return;
1398 }
1399 }

notifyd_apply_reclog() call call TALLOC_FREE on state->peers[i],
and in the destructor it will swap out the entry for
state->peers[i] with state->peers[i] = state->peers[state->num_peers-1];

That means in the log index does not match case we will
set state->peers[i]->last_broadcast = time(NULL) incorrectly,
as the last broadcast time of the deleted record.

I think the correct fix is to change notifyd_apply_reclog()
to return a bool, and just 'continue' the for loop if
it returns false.

Can you confirm this problem (it's complex code :-) ?

If so, do you want me to do the fix or do you want
to post another version ?

Looking forward to getting this code in very soon now,
it's been waiting long enough :-) :-).

Cheers,

Jeremy.

[Volker Lendecke]

Post by Jeremy Allison

Hi!
Attached find the latest incarnation of the notifyd I've
been talking about for a while now. Survived a few private
autobuilds.
Review appreciated!

Only issue I've found so far.

... Wow, you've really looked at the code. Thanks!!

Post by Jeremy Allison
notifyd_apply_reclog() call call TALLOC_FREE on state->peers[i],
and in the destructor it will swap out the entry for
state->peers[i] with state->peers[i] = state->peers[state->num_peers-1];
That means in the log index does not match case we will
set state->peers[i]->last_broadcast = time(NULL) incorrectly,
as the last broadcast time of the deleted record.
I think the correct fix is to change notifyd_apply_reclog()
to return a bool, and just 'continue' the for loop if
it returns false.
Can you confirm this problem (it's complex code :-) ?

Of course, fully correct. I'd solve it slighty differently: Just assign
the last_broadcast in apply_reclog.

Post by Jeremy Allison
If so, do you want me to do the fix or do you want
to post another version ?

Attached find two files: One with the changes just for review, and one
with the full new version.

Thanks!

Volker

[Jeremy Allison]

Post by Volker Lendecke

Post by Jeremy Allison

Hi!
Attached find the latest incarnation of the notifyd I've
been talking about for a while now. Survived a few private
autobuilds.
Review appreciated!

Only issue I've found so far.

... Wow, you've really looked at the code. Thanks!!

Post by Jeremy Allison
notifyd_apply_reclog() call call TALLOC_FREE on state->peers[i],
and in the destructor it will swap out the entry for
state->peers[i] with state->peers[i] = state->peers[state->num_peers-1];
That means in the log index does not match case we will
set state->peers[i]->last_broadcast = time(NULL) incorrectly,
as the last broadcast time of the deleted record.
I think the correct fix is to change notifyd_apply_reclog()
to return a bool, and just 'continue' the for loop if
it returns false.
Can you confirm this problem (it's complex code :-) ?

Of course, fully correct. I'd solve it slighty differently: Just assign
the last_broadcast in apply_reclog.

Post by Jeremy Allison
If so, do you want me to do the fix or do you want
to post another version ?

Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.

W00t! Amazing work, now in for 4.3.0 !!!!!

Thanks,

Jeremy.

[Volker Lendecke]

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

Volker

[Jeremy Allison]

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).

Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).

If so, get him to cancel my autobuild and schedule him to spend
some time on it.

Also, this code has been in production on a site for a long
time now, so this increases my confidence in it.

I will leave the call to you, but personally I'm good with
this code going into master and if we find some bugs later,
we'll just patch as normal.

[Michael Adam]

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).
If so, get him to cancel my autobuild and schedule him to spend
some time on it.
Also, this code has been in production on a site for a long
time now, so this increases my confidence in it.
I will leave the call to you, but personally I'm good with
this code going into master and if we find some bugs later,
we'll just patch as normal.

I am very happy to see this get in!

I have most probably not looked as thoroughly as Jeremy, but this
looks great! So I'd say just leave the autobuild in queue.
The "net notify" command will need some manpage update.
And there are a few typos and an indentation bug that caught
my eye in the comment with the ascii art for the daemon layout.
But that is just nit-picking.

Jeremy: If you ever want to cancel an autobuild you can:

- just push plain origin/master over the running or queued
autobuild
- alternatively, log in, and do "git fetch origin" followed
by "git reset --hard origin/master" in your autobuild
checkout dir.

Cheers - Michael

[Jeremy Allison]

Post by Michael Adam

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).
If so, get him to cancel my autobuild and schedule him to spend
some time on it.
Also, this code has been in production on a site for a long
time now, so this increases my confidence in it.
I will leave the call to you, but personally I'm good with
this code going into master and if we find some bugs later,
we'll just patch as normal.

I am very happy to see this get in!
I have most probably not looked as thoroughly as Jeremy, but this
looks great! So I'd say just leave the autobuild in queue.
The "net notify" command will need some manpage update.
And there are a few typos and an indentation bug that caught
my eye in the comment with the ascii art for the daemon layout.
But that is just nit-picking.
- just push plain origin/master over the running or queued
autobuild
- alternatively, log in, and do "git fetch origin" followed
by "git reset --hard origin/master" in your autobuild
checkout dir.

I already spoke to Volker on the phone. I'm not cancelling
the autobuild :-).

[Michael Adam]

Post by Jeremy Allison

Post by Michael Adam

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).
If so, get him to cancel my autobuild and schedule him to spend
some time on it.
Also, this code has been in production on a site for a long
time now, so this increases my confidence in it.
I will leave the call to you, but personally I'm good with
this code going into master and if we find some bugs later,
we'll just patch as normal.

I am very happy to see this get in!
I have most probably not looked as thoroughly as Jeremy, but this
looks great! So I'd say just leave the autobuild in queue.
The "net notify" command will need some manpage update.
And there are a few typos and an indentation bug that caught
my eye in the comment with the ascii art for the daemon layout.
But that is just nit-picking.
- just push plain origin/master over the running or queued
autobuild
- alternatively, log in, and do "git fetch origin" followed
by "git reset --hard origin/master" in your autobuild
checkout dir.

I already spoke to Volker on the phone. I'm not cancelling
the autobuild :-).

Good! These instructions were just for your reference
in case you need them in the future. ;)

Cheers - Michael

[Stefan (metze) Metzmacher]

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).

Looking briefly I found a bug in source3/wscript_build
if bld.env['dmapi_lib'] is not empty
the build fails with unknown dependency to 'dmfam', there's a missing
whitespace.

I'll continue tomorrow...

metze

[Jeremy Allison]

Post by Stefan (metze) Metzmacher

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).

Looking briefly I found a bug in source3/wscript_build
if bld.env['dmapi_lib'] is not empty
the build fails with unknown dependency to 'dmfam', there's a missing
whitespace.
I'll continue tomorrow...

Well if the autobuild goes through, just post patches
for the build system on top and I'll review.

Cheers,

Jeremy.

[Stefan (metze) Metzmacher]

Post by Jeremy Allison

Post by Stefan (metze) Metzmacher

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).

Looking briefly I found a bug in source3/wscript_build
if bld.env['dmapi_lib'] is not empty
the build fails with unknown dependency to 'dmfam', there's a missing
whitespace.
I'll continue tomorrow...

Well if the autobuild goes through, just post patches
for the build system on top and I'll review.

Ok, here's the fix.

Please review and push.

Thanks!
metze

[Michael Adam]

Post by Stefan (metze) Metzmacher

Post by Jeremy Allison

Post by Stefan (metze) Metzmacher

Post by Jeremy Allison

Post by Volker Lendecke

Post by Jeremy Allison

Post by Volker Lendecke
Attached find two files: One with the changes just for review, and one
with the full new version.

Pushed with minor change to "smbd: Remove SMB_VFS_NOTIFY_WATCH" commit
to add version number comment /* Version 33 - Remove notify_watch_fn */
to source3/include/vfs.h.
W00t! Amazing work, now in for 4.3.0 !!!!!

Wait -- can we get a second ack on this before we push? This
is too large a change for just a single reviewer! Not that I
don't trust you, but the bus factor is way too low for this
if only me and you have taken a look.

I did do the autobuild push already (not sure how to cancel that).
Personally I feel this is no larger than the spotlight code which
just went in with only 2 reviewers :-), and I'm guessing that what
you really mean here is you want metze to look :-).

Looking briefly I found a bug in source3/wscript_build
if bld.env['dmapi_lib'] is not empty
the build fails with unknown dependency to 'dmfam', there's a missing
whitespace.
I'll continue tomorrow...

Well if the autobuild goes through, just post patches
for the build system on top and I'll review.

Ok, here's the fix.
Please review and push.

pushed.

[Andreas Schneider]

Hi!

Hi Volker,

Attached find the latest incarnation of the notifyd I've
been talking about for a while now. Survived a few private
autobuilds.

this is really great work. Could you please write a text for WHATSNEW.txt and
send it to Karolin?


Thanks,


-- andreas