Re: [Qemu-devel] [PATCH 1/5] s390x/css: introduce css data stream

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 6 Sep 2017 14:40:48 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/06/2017 02:18 PM, Cornelia Huck wrote:
> > On Tue,  5 Sep 2017 13:16:41 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> +static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
> >> +                                  CcwDataStreamOp op)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = cds_check_len(cds, len);
> >> +    if (ret <= 0) {
> >> +        return ret;
> >> +    }
> >> +    if (op == CDS_OP_A) {
> >> +        goto incr;
> >> +    }
> >> +    ret = address_space_rw(&address_space_memory, cds->cda,
> >> +                           MEMTXATTRS_UNSPECIFIED, buff, len, op);
> >> +    if (ret != MEMTX_OK) {
> >> +        cds->flags |= CDS_F_STREAM_BROKEN;  
> > 
> > Do we want to distinguish between different reasons for the stream
> > being broken? I.e, is there a case where we want to signal different
> > errors back to the caller?
> >   
> 
> Hm, after I've done the error handling it seems that basically
> everything is to be handled with a program check. The stream
> records the place where the problem was encountered, so for debug
> we would not have to search for long.
> 
> There seems to be no need to distinguish.

OK, makes sense. Let's keep it as it is.

> 
> >> +        return -EINVAL;
> >> +    }
> >> +incr:
> >> +    cds->at_byte += len;
> >> +    cds->cda += len;
> >> +    return 0;
> >> +}
> >> +
> >> +void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> >> +{
> >> +    /*
> >> +     * We don't support MIDA (an optional facility) yet and we
> >> +     * catch this earlier. Just for expressing the precondition.
> >> +     */
> >> +    assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));  
> > 
> > I don't know, this is infrastructure, should it trust its callers? If
> > you keep the assert, please make it g_assert().  
> 
> 
> Why g_assert? I think g_assert comes form a test framework, this is not
> test code.

g_assert() is glib, no?

> 
> I feel more comfortable having this assert around.

Let's revisit that when we add mida support :) I don't really object to
it.

> 
> >   
> >> +    cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> >> +                 (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> >> +                 (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> >> +    cds->count = ccw->count;
> >> +    cds->cda_orig = ccw->cda;
> >> +    ccw_dstream_rewind(cds);
> >> +    if (!(cds->flags & CDS_F_IDA)) {
> >> +        cds->op_handler = ccw_dstream_rw_noflags;
> >> +    } else {
> >> +        assert(false);  
> > 
> > Same here.
> > 
> > Or should we make this return an error and have the callers deal with
> > that? (I still need to look at the users.)
> >   
> 
> This assert is going away soon (patch 4). I'm not sure doing much more here
> is justified.

OK, if it is transient anyway...