Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
On Tue, Oct 21, 2008 at 03:16:20PM -0400, Ben Guthro wrote: [PATCH 07/12] Domain Events - remote driver Deliver local callbacks in response to remote events remote_internal.c | 276 +- 1 file changed, 271 insertions(+), 5 deletions(-) ACK to this. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
Just discovered one tiny problem here - need to check 'event' to see
if the POLLHUP or POLLERR flags are set, and unregister the callback.
Otherwise if you kill the server, the client will just spin on POLLHUP
or ERR forever. Something like this ought todo the trick
if (event (POLLERR | POLLHUP)) {
virEventRemoveHandle(fd);
return;
}
I've been looking over the rest of your changes.
Generally, I agree all these suggestions are good ones...except for the code
above
With this code in, I run the following test
1. start libvirtd
2. begin to monitor events with event-test
3. virsh create foo.xml
At this point, the event-test app encounters a HUP, or ERR, and stops
monitoring for events - it will only ever get the Started event
I handle this in the event-test poll loop via
if ( pfd.revents POLLHUP ) {
DEBUG0(Reset by peer);
return -1;
}
Is it not a reasonable restriction to require the client app to handle a Hangup?
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
On Fri, Oct 17, 2008 at 12:02:13PM -0400, Ben Guthro wrote:
Deliver local callbacks in response to remote events
remote_internal.c | 255
--
1 file changed, 248 insertions(+), 7 deletions(-)
diff --git a/src/remote_internal.c b/src/remote_internal.c
index 35b7b4b..13537f7 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -34,6 +34,7 @@
+/** remoteDomainEventFired:
+ *
+ * The callback for monitoring the remote socket
+ * for event data
+ */
+void remoteDomainEventFired(int fd ATTRIBUTE_UNUSED,
+ int event ATTRIBUTE_UNUSED,
+ void *opaque)
+{
+char buffer[REMOTE_MESSAGE_MAX];
+char buffer2[4];
+struct remote_message_header hdr;
+XDR xdr;
+int len;
+
+virConnectPtrconn = opaque;
+struct private_data *priv = conn-privateData;
+
+DEBUG(%s : Event fired, __FUNCTION__);
+
+/* Read and deserialise length word. */
+if (really_read (conn, priv, 0, buffer2, sizeof buffer2) == -1)
+return;
Just discovered one tiny problem here - need to check 'event' to see
if the POLLHUP or POLLERR flags are set, and unregister the callback.
Otherwise if you kill the server, the client will just spin on POLLHUP
or ERR forever. Something like this ought todo the trick
if (event (POLLERR | POLLHUP)) {
virEventRemoveHandle(fd);
return;
}
before we try to read any data.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
On Mon, Oct 20, 2008 at 10:48:38AM -0400, Ben Guthro wrote:
Just discovered one tiny problem here - need to check 'event' to see
if the POLLHUP or POLLERR flags are set, and unregister the callback.
Otherwise if you kill the server, the client will just spin on POLLHUP
or ERR forever. Something like this ought todo the trick
if (event (POLLERR | POLLHUP)) {
virEventRemoveHandle(fd);
return;
}
I've been looking over the rest of your changes.
Generally, I agree all these suggestions are good ones...except for the code
above
With this code in, I run the following test
1. start libvirtd
2. begin to monitor events with event-test
3. virsh create foo.xml
At this point, the event-test app encounters a HUP, or ERR, and stops
monitoring for events - it will only ever get the Started event
That's a little odd - I'm not sure why 'event-test' would be getting
a HUP/ERR when 'virsh' starts a domain.
'event-test' should only get a HUP/ERR if it looses its socket connection
to the libvirtd server. Once you've lost the connection like this, the
entire virConnectPtr is non-operative, and the client app needs to
create a new virConnectPtr to re-connect. So removing the FD from the
event loop shouldn't result in us loosing any events - we're already
doomed there.to
I handle this in the event-test poll loop via
if ( pfd.revents POLLHUP ) {
DEBUG0(Reset by peer);
return -1;
}
The integration into the event loop though is only something the app
will have general control off - eg, in the example libvirt-glib code
I posted its totally opaque to the app.
Is it not a reasonable restriction to require the client app to handle a
Hangup?
Once the socket is dead, all subsequent libvirt call made on that
virConnectPtr object will throw errors, which the app should see.
Though if they're only ever waiting for events, and not making any
other calls, they'd not see it. Perhaps we could do with an explicit
callback for connection disconnect.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
OK - it looks like my EventImpl was passing along the wrong bits.
I'll look into the token scheme suggested in an earlier email, and get this
ready for re-submission.
Would you prefer a new patch series, as before - or another patch that modifies
the prior series?
Daniel P. Berrange wrote on 10/20/2008 10:59 AM:
On Mon, Oct 20, 2008 at 10:48:38AM -0400, Ben Guthro wrote:
Just discovered one tiny problem here - need to check 'event' to see
if the POLLHUP or POLLERR flags are set, and unregister the callback.
Otherwise if you kill the server, the client will just spin on POLLHUP
or ERR forever. Something like this ought todo the trick
if (event (POLLERR | POLLHUP)) {
virEventRemoveHandle(fd);
return;
}
I've been looking over the rest of your changes.
Generally, I agree all these suggestions are good ones...except for the code
above
With this code in, I run the following test
1. start libvirtd
2. begin to monitor events with event-test
3. virsh create foo.xml
At this point, the event-test app encounters a HUP, or ERR, and stops
monitoring for events - it will only ever get the Started event
That's a little odd - I'm not sure why 'event-test' would be getting
a HUP/ERR when 'virsh' starts a domain.
'event-test' should only get a HUP/ERR if it looses its socket connection
to the libvirtd server. Once you've lost the connection like this, the
entire virConnectPtr is non-operative, and the client app needs to
create a new virConnectPtr to re-connect. So removing the FD from the
event loop shouldn't result in us loosing any events - we're already
doomed there.to
I handle this in the event-test poll loop via
if ( pfd.revents POLLHUP ) {
DEBUG0(Reset by peer);
return -1;
}
The integration into the event loop though is only something the app
will have general control off - eg, in the example libvirt-glib code
I posted its totally opaque to the app.
Is it not a reasonable restriction to require the client app to handle a
Hangup?
Once the socket is dead, all subsequent libvirt call made on that
virConnectPtr object will throw errors, which the app should see.
Though if they're only ever waiting for events, and not making any
other calls, they'd not see it. Perhaps we could do with an explicit
callback for connection disconnect.
Daniel
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
On Mon, Oct 20, 2008 at 11:43:24AM -0400, Ben Guthro wrote: OK - it looks like my EventImpl was passing along the wrong bits. I'll look into the token scheme suggested in an earlier email, and get this ready for re-submission. Would you prefer a new patch series, as before - or another patch that modifies the prior series? Best to just re-post the whole series with changes incorporated, rather than trying to add more patches ontop of patches. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
