Re: [Xen-devel] [PATCH v5 RFC 09/14] tools/libxc: x86 PV restore code

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-06-12 at 18:01 +0100, Andrew Cooper wrote:
> On 12/06/14 16:49, David Vrabel wrote:
> > On 11/06/14 19:14, Andrew Cooper wrote:
> >> --- /dev/null
> >> +++ b/tools/libxc/saverestore/restore_x86_pv.c
> > ...
> >> +static int x86_pv_process_record(struct context *ctx, struct record *rec)
> >> +{
> >> +    xc_interface *xch = ctx->xch;
> >> +
> >> +    switch ( rec->type )
> >> +    {
> >> +    case REC_TYPE_X86_PV_INFO:
> >> +        return handle_x86_pv_info(ctx, rec);
> >> +
> > ...
> >> +
> >> +    case REC_TYPE_X86_PV_VCPU_MSRS:
> >> +        return handle_x86_pv_vcpu_msrs(ctx, rec);
> >> +
> >> +    default:
> >> +        if ( rec->type & REC_TYPE_OPTIONAL )
> >> +        {
> >> +            IPRINTF("Ignoring optional record (0x%"PRIx32", %s)",
> >> +                    rec->type, rec_type_to_str(rec->type));
> >> +            return 0;
> >> +        }
> >> +
> >> +        ERROR("Invalid record type (0x%"PRIx32", %s) for x86_pv domains",
> >> +              rec->type, rec_type_to_str(rec->type));
> >> +        return -1;
> >> +    }
> > I think this default case can be moved to common code.  Perhaps return
> > XC_SR_RECORD_UNHANDLED (== 1) to indicate the record wasn't handled.
> >
> > David
> 
> It am looking to not adversely affect any attempt to standardise libxc
> error reporting in the future, which is why all this code uses 0 or -1,
> even though errno might or might not be relevant.  Even at the moment,
> there are callbacks into higher level toolstacks which can return any
> arbitrary error.

That's fine but what David is suggesting is a sentinel value which is
entirely between the common code and the arch hook, both of which you
are defining here, it should never escape past that common caller
AFAICT.

Wanting to clean things up and not wanting to make that harder is fine,
but lets no hobble new code which could legitimately make good use of
their return value, AFAICT this is one of those cases.

Ian.


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