Xen devel - Aug 2010 - [PATCH] libxc: make unlock_page return error

Hi!

As a result of debugging ''xend segfaults when starting'',
the attached patch makes unlock_pages return an error.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return
error"):
> As a result of debugging ''xend segfaults when starting'',
> the attached patch makes unlock_pages return an error.
I''m not opposed to this general idea, but: you change unlock_pages to
return an int but you don''t seem to change any of its callers.  What
is the point ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wednesday 18 August 2010 18:05:40 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page
return
error"):
> > As a result of debugging ''xend segfaults when
starting'',
> > the attached patch makes unlock_pages return an error.
>
> I''m not opposed to this general idea, but: you change unlock_pages
to
> return an int but you don''t seem to change any of its callers.
That''s right. I leave this todo item to someone else.
> What is the point ?
In this shape it helps debugging problems like I do with Ian Campell.
It makes it easy to just add a debug output to print the error code.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxc: make unlock_page
return error"):
> On Wednesday 18 August 2010 18:05:40 Ian Jackson wrote:
> > What is the point ?
> 
> In this shape it helps debugging problems like I do with Ian Campell.
> It makes it easy to just add a debug output to print the error code.
Right.  OK.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page return
error"):
> As a result of debugging ''xend segfaults when starting'',
> the attached patch makes unlock_pages return an error.
Having read this in more detail, it seems that you''re making
{lock,unlock}_pages return errno values on error.  That''s fine but
it''s quite unusual in libxc; all libxc functions normally return -1 on
error and set errno.

So I think the unusual return value convention is worth a comment in
xc_private.h.  Can you please resubmit with such a comment ?

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-08-19 at 15:46 +0100, Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make unlock_page
return error"):
> > As a result of debugging ''xend segfaults when
starting'',
> > the attached patch makes unlock_pages return an error.
> 
> Having read this in more detail, it seems that you''re making
> {lock,unlock}_pages return errno values on error.  That''s fine but
> it''s quite unusual in libxc; all libxc functions normally return
-1 on
> error and set errno.
> 
> So I think the unusual return value convention is worth a comment in
> xc_private.h.  Can you please resubmit with such a comment ?
I think it should just follow the normal libxc convention instead of
commenting on the use of an unusual return value convention.

I also think that such a patch must include (or be part of a series
which) updates the callers to actually do something with the new return
value, or else it is somewhat pointless.

Given that this patch was only for debugging purposes for an issue which
is now resolved is there any need?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thursday 19 August 2010 16:57:21 Ian Campbell wrote:
> On Thu, 2010-08-19 at 15:46 +0100, Ian Jackson wrote:
> > Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make
unlock_page
return error"):
> > > As a result of debugging ''xend segfaults when
starting'',
> > > the attached patch makes unlock_pages return an error.
> >
> > Having read this in more detail, it seems that you''re making
> > {lock,unlock}_pages return errno values on error.  That''s
fine but
> > it''s quite unusual in libxc; all libxc functions normally
return -1 on
> > error and set errno.
> >
> > So I think the unusual return value convention is worth a comment in
> > xc_private.h.  Can you please resubmit with such a comment ?
>
> I think it should just follow the normal libxc convention instead of
> commenting on the use of an unusual return value convention.
>
> I also think that such a patch must include (or be part of a series
> which) updates the callers to actually do something with the new return
> value, or else it is somewhat pointless.
>
> Given that this patch was only for debugging purposes for an issue which
> is now resolved is there any need?
The need I see for now is a future regression.

Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-08-19 at 16:39 +0100, Christoph Egger
wrote:
> On Thursday 19 August 2010 16:57:21 Ian Campbell wrote:
> > On Thu, 2010-08-19 at 15:46 +0100, Ian Jackson wrote:
> > > Christoph Egger writes ("[Xen-devel] [PATCH] libxc: make
unlock_page
> return error"):
> > > > As a result of debugging ''xend segfaults when
starting'',
> > > > the attached patch makes unlock_pages return an error.
> > >
> > > Having read this in more detail, it seems that you''re
making
> > > {lock,unlock}_pages return errno values on error. 
That''s fine but
> > > it''s quite unusual in libxc; all libxc functions
normally return -1 on
> > > error and set errno.
> > >
> > > So I think the unusual return value convention is worth a comment
in
> > > xc_private.h.  Can you please resubmit with such a comment ?
> >
> > I think it should just follow the normal libxc convention instead of
> > commenting on the use of an unusual return value convention.
> >
> > I also think that such a patch must include (or be part of a series
> > which) updates the callers to actually do something with the new
return
> > value, or else it is somewhat pointless.
> >
> > Given that this patch was only for debugging purposes for an issue
which
> > is now resolved is there any need?
> 
> The need I see for now is a future regression.
But if nothing checks the return value what use is it?

If it''s just for your debug patch you can always apply this same thing
as part of that debug patch, there''s no need for it to be upstream.

Ian.
> 
> Christoph
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

At 17:01 +0100 on 19 Aug (1282237291), Ian Campbell
wrote:
> > > Given that this patch was only for debugging purposes for an
issue which
> > > is now resolved is there any need?
> > 
> > The need I see for now is a future regression.
> 
> But if nothing checks the return value what use is it?
> 
> If it''s just for your debug patch you can always apply this same
thing
> as part of that debug patch, there''s no need for it to be
upstream.
+1.  Any future bug-hunter who needs this value can trivially add it,
and as it stands it''s just increasing the number of unchecked return
values in the codebase.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel