[Libvir] Problem with hypercall

[Daniel P. Berrange]

On Thu, Sep 28, 2006 at 11:37:42PM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 28, 2006 at 05:22:08PM -0400, Daniel Veillard wrote:
> > On Thu, Sep 28, 2006 at 09:43:19PM +0100, Daniel P. Berrange wrote:
> > > This is really quite a nasty problem, because the struct is passed into from
> > > numerous locations in the xen_internal.h code & I didn't want to cover the
> > > entire source with conditionals.
> > > 
> > > So what I've done is declared a new xen_v2_domaininfo struct which is
> > > the same as xen_v0_domaininfo, but with Jim's patch reverted again.
> > > Then provide two unions
> > 
> >   Okay, I really expected they didn't broke that data structure too,
> > sigh ... Yeah go ahead that seems the less uglier possible !
> 
> Ok, I comitted this to CVS now.
> 
> > I wonder why I didn't catch that, maybe my 3.0.3 test was done on an x86_64,
> 
> Luck. On one of my 32-bit 3.0.3 boxes the 0.1.6 appeared to work fine for the
> basic operations - it was just giving bogus data (eg, 153 cpus). Depending on
> how many domains were running & what their data was things either silently
> worked (but bogus data), or completely crashed & burned.
> 
> Anyway, I've now tested this on both versions of HV, and run through valgrind
> too as an extra sanity check. Lets just hope Xen doesn't break ABI yet again
> in the future....

Updating for this ABI breakage is beginning to feel like a never ending
problem :-(  It seems I missed one change in my last patch - mlock()'ing
the wrong bit of data for getdomainlist. By (bad) luck it just happened
to work anyway on my tests because I guess there was sufficient allocated
memory that mlock() was still in range. Running unprivileged via the proxy
though, the proxy will always request info on 1020 domains (even if only
a couple are running), which meant we mlock() several 10's of KB of data.
THis ran in an area on unallocated mem & caused mlock() to fail, hence
my discovering the problem.

Anyway, attached is the patch I've applied to CVS...

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  -=| 
-------------- next part --------------
Index: xen_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xen_internal.c,v
retrieving revision 1.43
diff -u -r1.43 xen_internal.c
--- xen_internal.c	29 Sep 2006 16:12:08 -0000	1.43
+++ xen_internal.c	2 Oct 2006 19:25:01 -0000
@@ -145,7 +145,15 @@
      domlist.v0[n].domain :                         \
      domlist.v2[n].domain)
 
+#define XEN_GETDOMAININFOLIST_DATA(domlist)     \
+    (hypervisor_version < 2 ?                   \
+     (void*)(domlist->v0) :                     \
+     (void*)(domlist->v2))
 
+#define XEN_GETDOMAININFO_SIZE                  \
+    (hypervisor_version < 2 ?                   \
+     sizeof(xen_v0_getdomaininfo) :             \
+     sizeof(xen_v2_getdomaininfo))
 
 #define XEN_GETDOMAININFO_CLEAR(dominfo)                        \
     (hypervisor_version < 2 ?                                   \
@@ -645,7 +653,8 @@
 {
     int ret = -1;
 
-    if (mlock(dominfos, sizeof(xen_v0_getdomaininfo) * maxids) < 0) {
+    if (mlock(XEN_GETDOMAININFOLIST_DATA(dominfos),
+              XEN_GETDOMAININFO_SIZE * maxids) < 0) {
         virXenError(VIR_ERR_XEN_CALL, " locking",
                     sizeof(xen_v0_getdomaininfo) * maxids);
         return (-1);
@@ -687,7 +696,8 @@
         if (ret == 0)
             ret = op.u.getdomaininfolist.num_domains;
     }
-    if (munlock(dominfos, sizeof(xen_v0_getdomaininfo) * maxids) < 0) {
+    if (mlock(XEN_GETDOMAININFOLIST_DATA(dominfos),
+              XEN_GETDOMAININFO_SIZE * maxids) < 0) {
         virXenError(VIR_ERR_XEN_CALL, " release",
                     sizeof(xen_v0_getdomaininfo));
         ret = -1;