On Mon, 16 Feb 2026 at 08:58, Shameer Kolothum Thodi <[email protected]> wrote: > > > > > -----Original Message----- > > From: Nicolin Chen <[email protected]> > > Sent: 13 February 2026 22:19 > > To: Shameer Kolothum Thodi <[email protected]> > > Cc: [email protected]; [email protected]; > > [email protected]; [email protected]; Nathan Chen > > <[email protected]>; Matt Ochs <[email protected]>; Jiandi An > > <[email protected]>; Jason Gunthorpe <[email protected]>; > > [email protected]; [email protected]; > > [email protected]; Krishnakant Jaju <[email protected]> > > Subject: Re: [PATCH v6 3/5] hw/arm/smmuv3-accel: Allocate vEVENTQ for > > accelerated SMMUv3 devices > > > > On Fri, Feb 13, 2026 at 10:39:40AM +0000, Shameer Kolothum wrote: > > > static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset, > > > uint64_t data, MemTxAttrs attrs) { > > > - Error *local_err = NULL; > > > + Error *err = NULL, *local_err = NULL; > > > > > > switch (offset) { > > > case A_CR0: > > > s->cr[0] = data; > > > s->cr0ack = data & ~SMMU_CR0_RESERVED; > > > /* in case the command queue has been enabled */ > > > - smmuv3_cmdq_consume(s, &local_err); > > > + smmuv3_cmdq_consume(s, &err); > > > + /* Allocate vEVENTQ if EVENTQ is enabled and a vIOMMU is > > > available > > */ > > > + smmuv3_accel_alloc_veventq(s, &local_err); > > > + error_propagate(&err, local_err); > > > > It would be cleaner to have: > > > > Error *local_err2 = NULL; > > Error *local_err = NULL; > > Yeah. I had a bit of back and forth on this. My thinking was to use err as > an accumulating error pointer in case additional error paths are added > later, with local_err_x used for intermediate calls. > > Either way is fine I guess for now.
Generally speaking we should try to stick to one of the defined standard patterns for Error handling in the big comment in include/qapi/error.h. Yours is the "receive and accumulate multiple errors" pattern, I think? That works, but do you really want to throw away the second error message without reporting it if both calls fail? Plus, from the commit message: > Also, rename the local error variable and refactor smmu_writel() to use > a single error accumulator with error_propagate(). If you find yourself writing "Also, ..." in a commit message, it's often a sign that you should consider splitting the commit in two. thanks -- PMM
