On Tue, Jan 19, 2016 at 01:38:53PM +0000, Cristian Stoica wrote:
> On 01/18/2016 11:04 PM, Phil Sutter wrote:
> > On Tue, Jan 12, 2016 at 12:08:02PM +0200, Cristian Stoica wrote:
> ...
> >> -  *src_sg = NULL; /* default to no input */
> >> -  *dst_sg = NULL; /* default to ignore output */
> >> -
> >
> > This does not look correct to me. The purpose of those is to ensure
> > *src_sg and *dst_sg are initialized. Otherwise if src or dst are NULL
> > (which matches the situation of "NULL input or output" as you state),
> > they are left uninitialized by this function.
> > Do you have an actual use case where this bites you?
> The use-case I'm thinking is one where get_userbuf is called knowing upfront 
> that
> only one of src or dst are of interest, skipping work completely on the other 
> buffer.
> Of course, I can call get_userbuf with both output scatterlists and ignore 
> one of
> them. But I would add more flexibility to this function if it is possible.
> For example:
> ret = get_userbuf(ses_ptr, src, len, NULL, 0, task, mm, &src_sg, NULL);
>                                       ^^^^  ^                     ^^^^
> The current implementation does not permit output &scatterlist to be NULL 
> even though
> it allows for input buffer to be NULL.

But it also doesn't need to, because get_userbuf() is never called this
way. In my opinion you're optimizing for a use-case which doesn't exist.
Once there is one, I'm all open for changes in that function, but until
that's the case I don't see sense in changing that function's behaviour.

> If this patch is incorrect for all cases, I'm thinking of some alternative 
> implementations:
> - the callers set the scatterlists  with NULL before calling get_userbuf
> - get_userbuf checks for non-NULL output scatterlists before writing default 
> values to them

It doesn't need to. The caller always just passes a pointer and expects
it to be initialized, either to NULL or a scatterlist which points to
the data src/dst points to. This is the determining information, and it
does the right thing in any case.

I really have no idea where you're heading at with this. But without
further context, it doesn't make any sense to me. If you have a
functional enhancement up your sleeve, I suggest submitting that along
with this change so I get an idea of what this is all about.

Cheers, Phil

Cryptodev-linux-devel mailing list

Reply via email to