Re: [PATCH v3] tpm: add support for partial reads

[Jarkko Sakkinen]

On Thu, Nov 15, 2018 at 04:26:33PM -0800, Tadeusz Struk wrote:
> On 11/15/18 3:31 PM, Jarkko Sakkinen wrote:
> > You could drop these memset() calls and also one from
> > tpm_timeout_work(). The call could be done once in the beginning of
> > tpm_common_write() instead of having three different call sites.
> > 
> 
> Don't we want to clean the buffer as the response is read?
> When we will only memset it in write and if one would send
> just one command there will only be one response.
> The data will sit in the buffer until the next command.
> There will be a quite bit time window when the data can leak.

Point taken.

> > Naming becomes a mess and the comment for data_pending variable is
> > incorrect as it is also used for synchronous operation.
> > 
> > Maybe add a prepending commit to rename it as
> > 
> > 	/* Holds the resul of the tpm_transmit() last call. */
> > 	ssize_t transmit_result;
> 
> Agree, will do that.
> 
> > 
> > That is at least clear and obvious on what it contains.
> > 
> > The comment for partial_data is incorrect as the variable does not
> > contain any data.
> 
> Yes, I will change it.
> 
> > 
> > We could use declare:
> > 
> > 	/* Holds the count how much of the response is still unread. */
> > 	size_t response_pending;
> > 
> > Observe another remark from your commit: there is no reaso to ssize_t as
> > the type as the value should never be a negative number.
> 
> Yes, in fact now the priv->data_pending can also be only positive.
> I'll change it and send a new version soon.

Right you're correct. Then it should be simply called as response_size
(and be size_t) as it is then the most precise name for what it
contains.

> Thanks,
> -- 
> Tadeusz

/Jarkko