9:26 a.m.
The patches that I'm going to follow up with change virDomainLookup*
functions so that they raise VIR_ERR_NO_DOMAIN where a domain doesn't
exist (but there is no other error), else raise some other error where
there's an actual error occuring.
This will allow wrappers in other languages to distinguish the three
cases found / not found / error.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
10:12 a.m.
New subject: [Libvir] [PATCH] #1: Add error handling and do quick wins (qemu, test)
This patch:
(1) Adds the VIR_ERR_NO_DOMAIN and VIR_ERR_NO_NETWORK errors (the latter
will be used later to make the same change to virNetworkLookup* functions).
(2) Changes the documentation of virDomainLookup* functions to match the
new behaviour.
(3) Changes qemu & test drivers to have the new behaviour.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:05 p.m.
New subject: [Libvir] [PATCH] #1: Add error handling and do quick wins (qemu, test)
On Tue, Jul 03, 2007 at 04:12:20PM +0100, Richard W.M. Jones wrote:
This patch:
(1) Adds the VIR_ERR_NO_DOMAIN and VIR_ERR_NO_NETWORK errors (the latter
will be used later to make the same change to virNetworkLookup* functions).
(2) Changes the documentation of virDomainLookup* functions to match the
new behaviour.
(3) Changes qemu & test drivers to have the new behaviour.
Looks good.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
5:20 a.m.
New subject: [Libvir] [PATCH] #1: Add error handling and do quick wins (qemu, test)
On Tue, Jul 03, 2007 at 04:12:20PM +0100, Richard W.M. Jones wrote:
This patch:
(1) Adds the VIR_ERR_NO_DOMAIN and VIR_ERR_NO_NETWORK errors (the latter
will be used later to make the same change to virNetworkLookup* functions).
(2) Changes the documentation of virDomainLookup* functions to match the
new behaviour.
(3) Changes qemu & test drivers to have the new behaviour.
Looks fine, +1
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
10:12 a.m.
New subject: [Libvir] [PATCH] #2: Fix remote so that EPIPE errors are caught and reported
Here we just fix the remote driver so that if the server goes down, the
process doesn't die on SIGPIPE.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:10 p.m.
New subject: [Libvir] [PATCH] #2: Fix remote so that EPIPE errors are caught and reported
On Tue, Jul 03, 2007 at 04:12:56PM +0100, Richard W.M. Jones wrote:
Here we just fix the remote driver so that if the server goes down,
the
process doesn't die on SIGPIPE.
This isn't thread safe. Changing signals with sigaction affects the
entire process - so if you've got multiple threads running with
multiple remote connections open, you can get overlapping calls.
Either we should just block sigpipe in virInitialize, or mandate
that the application needs to block sigpipe itself. I'm not sure
really what best course is - I'd probably lean towards saying it is
the application's responsibility to deal with sig handlers.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
5:25 a.m.
New subject: [Libvir] [PATCH] #2: Fix remote so that EPIPE errors are caught and reported
Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:12:56PM +0100, Richard W.M. Jones wrote:
> Here we just fix the remote driver so that if the server goes down, the
> process doesn't die on SIGPIPE.
This isn't thread safe. Changing signals with sigaction affects the
entire process - so if you've got multiple threads running with
multiple remote connections open, you can get overlapping calls.
Either we should just block sigpipe in virInitialize, or mandate
that the application needs to block sigpipe itself. I'm not sure
really what best course is - I'd probably lean towards saying it is
the application's responsibility to deal with sig handlers.
Yes, you're right. I'll change this so that instead it adds some
supporting documentation for app writers.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
10:14 a.m.
New subject: [Libvir] [PATCH] #3: Fix xm_internal / remote interaction
I'm not quite sure what the problem is (although the problem is in
xm_internal), but when you use xm_internal over remote, it sometimes
doesn't initialize its internal cache correctly, so it thinks that
there are no inactive domains.
The fix is a one-liner which I hit upon by accident -- I don't really
understand why it works:
@@ -489,7 +487,7 @@
xenXMOpen (virConnectPtr conn ATTRIBUTE_UNUSED,
const char *name ATTRIBUTE_UNUSED, int flags
ATTRIBUTE_UNUSED)
{
- if (nconnections == 0) {
+ if (configCache == NULL) {
configCache = virHashCreate(50);
if (!configCache)
return (-1);
But the attached patch also adds proper error messages to
xenXMConfigCacheRefresh too.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:12 p.m.
New subject: [Libvir] [PATCH] #3: Fix xm_internal / remote interaction
On Tue, Jul 03, 2007 at 04:14:30PM +0100, Richard W.M. Jones wrote:
I'm not quite sure what the problem is (although the problem is
in
xm_internal), but when you use xm_internal over remote, it sometimes
doesn't initialize its internal cache correctly, so it thinks that
there are no inactive domains.
The fix is a one-liner which I hit upon by accident -- I don't really
understand why it works:
Very peculiar - the nconnections stuff is incremented / decremented
by the xenXMOpen & xenXMClose methods. So the change you show below
should be identical to previous behaviour. Is something calling the
xenXMClose method too many times maybe ? I guess some judicious use
of syslog would show it up
@@ -489,7 +487,7 @@
xenXMOpen (virConnectPtr conn ATTRIBUTE_UNUSED,
const char *name ATTRIBUTE_UNUSED, int flags
ATTRIBUTE_UNUSED)
{
- if (nconnections == 0) {
+ if (configCache == NULL) {
configCache = virHashCreate(50);
if (!configCache)
return (-1);
But the attached patch also adds proper error messages to
xenXMConfigCacheRefresh too.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
3:16 a.m.
New subject: [Libvir] [PATCH] #3: Fix xm_internal / remote interaction
Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:14:30PM +0100, Richard W.M. Jones wrote:
> I'm not quite sure what the problem is (although the problem is in
> xm_internal), but when you use xm_internal over remote, it sometimes
> doesn't initialize its internal cache correctly, so it thinks that
> there are no inactive domains.
>
> The fix is a one-liner which I hit upon by accident -- I don't really
> understand why it works:
Very peculiar - the nconnections stuff is incremented / decremented
by the xenXMOpen & xenXMClose methods. So the change you show below
should be identical to previous behaviour. Is something calling the
xenXMClose method too many times maybe ? I guess some judicious use
of syslog would show it up
But I can assure you that the patch is necessary, even if I don't
understand how it works ...
Rich,
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
5:35 a.m.
New subject: [Libvir] [PATCH] #3: Fix xm_internal / remote interaction
On Wed, Jul 04, 2007 at 05:12:53AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:14:30PM +0100, Richard W.M. Jones wrote:
> I'm not quite sure what the problem is (although the problem is in
> xm_internal), but when you use xm_internal over remote, it sometimes
> doesn't initialize its internal cache correctly, so it thinks that
> there are no inactive domains.
>
> The fix is a one-liner which I hit upon by accident -- I don't really
> understand why it works:
Very peculiar - the nconnections stuff is incremented / decremented
by the xenXMOpen & xenXMClose methods. So the change you show below
should be identical to previous behaviour. Is something calling the
xenXMClose method too many times maybe ? I guess some judicious use
of syslog would show it up
Hum, I really dislike the following:
int xenXMClose(virConnectPtr conn ATTRIBUTE_UNUSED) {
if (!nconnections--) {
I don't want to have to rely on C operator priorities to try to guess
if the code is correct.
nconnections--;
if (nconnections <= 0) {
is way cleaner IMHO.
>
> @@ -489,7 +487,7 @@
> xenXMOpen (virConnectPtr conn ATTRIBUTE_UNUSED,
> const char *name ATTRIBUTE_UNUSED, int flags
> ATTRIBUTE_UNUSED)
> {
> - if (nconnections == 0) {
> + if (configCache == NULL) {
> configCache = virHashCreate(50);
> if (!configCache)
> return (-1);
IMHO that patch is wrong because it would not reset nconnections to 0
I would prefer to understand what is really going on. Could you check with
the cleanup of xenXMClose to see if this is sufficient ?
> But the attached patch also adds proper error messages to
> xenXMConfigCacheRefresh too.
That is fine. There is still one thing I don't understand in the original
patch, the replacement of xenDaemonDomainLookupByName() by
xenDaemonLookupByName() which I don't see in the current code ?!?
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
10:19 a.m.
New subject: [Libvir] [PATCH] #4: Change interface between xen_unified & its underlying drivers
If you recall the current driver model, which looks like this:
libvirt.c
|
(virDriver)
|
V
+---- xen_unified.c
|
(virDriver)
|
V
+---- xen_internal.c
|
+---- xend_internal.c
etc.
The interface between libvirt.c and xen_unified.c is virDriver. And
currently the same interface is reused between xen_unified.c and its
underlying drivers.
This patch defines a new structure called xenUnifiedDriver, which begins
as a subset of virDriver, and eventually will go away in favour of
direct calls from xen_unified to the underlying parts (much simpler to
understand what's going on that way, and we won't need the current
driver ordering hacks, and anyway the xen drivers call each other
semi-randomly).
libvirt.c
|
(virDriver)
|
V
+---- xen_unified.c
|
(xenUnifiedDriver + direct calls)
|
V
+---- xen_internal.c
|
+---- xend_internal.c
etc.
This patch starts by removing the id, name and version fields from
virDriver.
It also removes getMaxVcpus and the domainLookup* fields, which will
make more sense when you see patches #6 and #7 in this series.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:24 p.m.
New subject: [Libvir] [PATCH] #4: Change interface between xen_unified & its underlying drivers
On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
This patch starts by removing the id, name and version fields from
virDriver.
It also removes getMaxVcpus and the domainLookup* fields, which will
make more sense when you see patches #6 and #7 in this series.
Yes, i guess 4, 6 & 7 should really be all one patch - since if you
apply , but not 6 & 7 the driver is non-functional. Anyway, the combo
of this & the other patches look like they're doing what I'd expect,
so objections here...
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6:09 a.m.
New subject: [Libvir] [PATCH] #4: Change interface between xen_unified & its underlying drivers
On Wed, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
>
> This patch starts by removing the id, name and version fields from
> virDriver.
>
> It also removes getMaxVcpus and the domainLookup* fields, which will
> make more sense when you see patches #6 and #7 in this series.
Yes, i guess 4, 6 & 7 should really be all one patch - since if you
apply , but not 6 & 7 the driver is non-functional. Anyway, the combo
of this & the other patches look like they're doing what I'd expect,
so objections here...
Honnestly I'm not really convinced as such the patches are an improvement.
You introduce a new structure, it's still indirect, how is that better ?
Now if you want to get rid of the indirection, why not but:
- this forces to make all those functions public again
- puts more logic in xen_unified, I'm not sure its code would
become more maintainable
This would probably help in debugging sessions, that's right.
Do we want to do this while Xen support is still in flux, we need
to do Xen-API support (on localhost, I would not extend over network)
in a not so distant future, the driver based approach simplifies the
integration of new code.
I'm not against applying those 3 patches but at the moment I still
feel unconvinced :-)
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6:21 a.m.
New subject: [Libvir] [PATCH] #4: Change interface between xen_unified & its underlying drivers
Daniel Veillard wrote:
On Wed, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
> On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
>> This patch starts by removing the id, name and version fields from
>> virDriver.
>>
>> It also removes getMaxVcpus and the domainLookup* fields, which will
>> make more sense when you see patches #6 and #7 in this series.
> Yes, i guess 4, 6 & 7 should really be all one patch - since if you
> apply , but not 6 & 7 the driver is non-functional. Anyway, the combo
> of this & the other patches look like they're doing what I'd expect,
> so objections here...
Honnestly I'm not really convinced as such the patches are an improvement.
You introduce a new structure, it's still indirect, how is that better ?
I didn't explain it well, but here's why: At the moment if you want to
find out how, say, "reboot" is implemented you need to first of all
examine all 5 underlying drivers to see which have a non-NULL function
in that slot in the driver table. Secondly you need to examine
xen_unified.c in two places: at the top of the file to see what order
the backend drivers are normally called in, then at the
xenUnifiedDomainReboot function itself to see if the function follows
that order or is one of the exceptions that does its own thing. So you
have to examine 7 places in the code. I want to bring that all together
so that you only have to examine one place (xenUnifiedDomainReboot) to
see how it is implemented.
The fact that at the moment virDriver is a dual-purpose structure was
just about convenience when I originally implemented xen-unified. It
makes no particular sense to have the interface between libvirt.c and
drivers be the same as the xen-internal interface between xen_unified.c
and its underlying drivers.
Now if you want to get rid of the indirection, why not but:
- this forces to make all those functions public again
In dynamic libs these functions will be hidden because of the
libvirt_syms file. In static libvirt.a the functions will be exported,
but they are already exported at the moment via the driver table, so
there is no practical difference.
- puts more logic in xen_unified, I'm not sure its code would
become more maintainable
This would probably help in debugging sessions, that's right.
Do we want to do this while Xen support is still in flux, we need
to do Xen-API support (on localhost, I would not extend over network)
in a not so distant future, the driver based approach simplifies the
integration of new code.
I really don't think the patch as it stands is a major change though - I
mean the calls are still going through a structure, it just happens to
be a xen-specific structure rather than the virDriver structure.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
11:41 a.m.
New subject: [Libvir] [PATCH] #4: Change interface between xen_unified & its underlying drivers
On Wed, Jul 04, 2007 at 12:21:18PM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
>On Wed, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
>>On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
>>>This patch starts by removing the id, name and version fields from
>>>virDriver.
>>>
>>>It also removes getMaxVcpus and the domainLookup* fields, which will
>>>make more sense when you see patches #6 and #7 in this series.
>>Yes, i guess 4, 6 & 7 should really be all one patch - since if you
>>apply , but not 6 & 7 the driver is non-functional. Anyway, the combo
>>of this & the other patches look like they're doing what I'd expect,
>>so objections here...
>
> Honnestly I'm not really convinced as such the patches are an
> improvement.
>You introduce a new structure, it's still indirect, how is that better ?
I didn't explain it well, but here's why: At the moment if you want to
find out how, say, "reboot" is implemented you need to first of all
examine all 5 underlying drivers to see which have a non-NULL function
in that slot in the driver table. Secondly you need to examine
xen_unified.c in two places: at the top of the file to see what order
the backend drivers are normally called in, then at the
xenUnifiedDomainReboot function itself to see if the function follows
that order or is one of the exceptions that does its own thing. So you
have to examine 7 places in the code. I want to bring that all together
so that you only have to examine one place (xenUnifiedDomainReboot) to
see how it is implemented.
Hum, okay :-)
+1
but ultimately the real fix is to drop that sub driver structure.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
10:19 a.m.
New subject: [Libvir] [PATCH] #5: Tidy up whitespace in xs_internal.c
Fix the whitespace in xs_internal.c xenStoreLookupByName function.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
5:46 a.m.
New subject: [Libvir] [PATCH] #5: Tidy up whitespace in xs_internal.c
On Tue, Jul 03, 2007 at 04:19:31PM +0100, Richard W.M. Jones wrote:
Fix the whitespace in xs_internal.c xenStoreLookupByName function.
sure, +1
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
10:20 a.m.
New subject: [Libvir] [PATCH] #6: Change xenUnifiedGetMaxVcpus to make direct calls to the underlying driver
Because the type field is now missing from xenUnifiedDriver, the old
method of implementing xenUnifiedGetMaxVcpus didn't work. There's only
one underlying driver which can implement this anyway, so just make the
call directly.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6:53 a.m.
New subject: [Libvir] [PATCH] #6: Change xenUnifiedGetMaxVcpus to make direct calls to the underlying driver
Richard W.M. Jones wrote:
diff -u -p -r1.14 xen_unified.c
--- src/xen_unified.c 26 Jun 2007 11:42:46 -0000 1.14
+++ src/xen_unified.c 3 Jul 2007 15:01:39 -0000
@@ -276,17 +269,17 @@ xenUnifiedGetURI (virConnectPtr conn)
static int
xenUnifiedGetMaxVcpus (virConnectPtr conn, const char *type)
{
- GET_PRIVATE(conn);
Not quite sure what happened there, but obviously the above call to
GET_PRIVATE needs to stay because the priv pointer is used in the
subsequent code ...
+ if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET])
+ return xenHypervisorGetMaxVcpus (conn, type);
+ else {
+ xenUnifiedError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
+ return -1;
+ }
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
10:20 a.m.
New subject: [Libvir] [PATCH] #7: Change the virDomainLookup* calls
This changes the three virDomainLookup* calls to make direct calls to
the underlying drivers, and to return VIR_ERR_NO_DOMAIN error if the
domain is not found.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6381
days inactive
6382
days old
20 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones