Re: [PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks

[George Dunlap <george.dunlap@citrix.com>]

On 29/02/16 16:58, George Dunlap wrote:
> On 26/02/16 12:30, Ian Jackson wrote:
>> George Dunlap writes ("[PATCH 6/8] tools/xenalyze: Fix off-by-one in MAX_CPUS range checks"):
>>> Skip action / throw error if cpu/vcpu >= MAX_CPUS  rather than >.
>>>
>>> Also add an assertion to vcpu_find, to make future errors of this kind
>>> not out-of-bounds.
>> ...
>>> +    /* "Graceful" handling of vid >= MAX_CPUS should be handled elsewhere */
>>> +    if ( vid >= MAX_CPUS ) {
>>> +        fprintf(stderr, "%s: vcpu %d exceeds MAX_CPUS %d!\n",
>>> +                __func__, vid, MAX_CPUS);
>>> +        error(ERR_ASSERT, NULL);
>>> +    }
>>
>> I'm not convinced by the existence of error(ERR_ASSERT,...).  What is
>> wrong with assert() ?
> 
> Well one half of the reason for error() in general is to print out the
> record which caused (or was involved in) the error before dying.  And
> I'm guessing that once I decided I'd have error(ERR_ASSERT, xxx), that
> for consistency I just decided to use error(ERR_ASSERT,...) everywhere.

Oh, actually -- going through and implementing this change, I *think*
that the problem I had was actually that assert() doesn't flush stdout
before calling abort().  In dump mode every single trace record is
printed to stdout, which makes it fairly easy to figure out how you go
to the point of the assertion -- as long as it's actually printed out.

In fact in one location I had commented out an assert() and replaced it
with an if() {fprintf(...) error(...)}, presumably for exactly that reason.

In the case of xenalyze, all the recent trace records after an error
message is actually a lot more useful for forensics than having the
stack trace (which is what abort() gives you).

 -George


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