Quoting Tvrtko Ursulin (2019-03-14 17:58:00)
>
> On 14/03/2019 17:15, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-14 16:47:05)
> >>
> >> On 13/03/2019 14:43, Chris Wilson wrote:
> >>> +static int clone_engines(struct i915_gem_context *dst,
> >>> + struct i915_gem_context *src)
> >>> +{
> >>> + struct intel_engine_cs **engines;
> >>> + unsigned int nengine;
> >>> +
> >>> + mutex_lock(&src->i915->drm.struct_mutex); /* serialise src->engine[] */
> >>> + nengine = src->nengine;
> >>> + if (!ZERO_OR_NULL_PTR(src->engines))
> >>
> >> A comment taking note of the reason for the ZERO_PTR trick.
> >
> > I must have deleted /* XXX why kmemdup, why? */
>
> Indeed why kmemdup? :) I thought it was about allowing count = 0 in the
> set_engines and storing ZERO_PTR in ctx->engines in that case. In which
> case it fall through the else branch and copies over the ZERO_PTR.
> Unless you wanted to complain that kmemdup does not handle ZERO_PTR in
> the source?
My first reaction was that kmemdup would indeed take care of the
ZERO_SIZE_PTR. I still think it should and should send a patch so that
it does.
> >>> @@ -110,6 +112,8 @@ struct i915_gem_context {
> >>> #define CONTEXT_CLOSED 1
> >>> #define CONTEXT_FORCE_SINGLE_SUBMISSION 2
> >>>
> >>> + unsigned int nengine;
> >>
> >> Can I tempt you to pluralise this? I at least find it more obvious in
> >> plural.
> >
> > Bah, historical precedent is
> > int nengine;
> > void *engines;
>
> Not everything historical is good! :)
I guess you want a num_ as well!
> >>> +static inline bool
> >>> +__check_struct_size(size_t base, size_t arr, size_t count, size_t *size)
> >>> +{
> >>> + size_t sz;
> >>> +
> >>> + if (check_mul_overflow(count, arr, &sz))
> >>> + return false;
> >>> +
> >>> + if (check_add_overflow(sz, base, &sz))
> >>> + return false;
> >>> +
> >>> + *size = sz;
> >>> + return true;
> >>> +}
> >>> +
> >>> +#define check_struct_size(T, A, C, SZ) \
> >>> + likely(__check_struct_size(sizeof(*(T)), \
> >>> + sizeof(*(T)->A) + __must_be_array((T)->A), \
> >>> + C, SZ))
> >>
> >> A comment explaining usage would be nice.
> >
> > /* see struct_size() */
>
> Come on, a bit more than that! Look at those self-documenting args, T,
> A, C, SZ! It needs to say what it will calculate, where it stores it and
> what it returns. If you want to have more hope people will actually use
> the helpers you add.
That alternative is to use
check_struct_size() (struct_size() < SIZE_MAX)
I didn't like that as it's conflating the overflow error and hope that
the compiler would be smarter with the overflow checking distinct.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
