Re: [PATCH v4 2/2] tpm: add support for partial reads

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Tadeusz Struk <tadeusz.struk@intel.com>
Cc: jgg@ziepe.ca, linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] tpm: add support for partial reads
Date: Mon, 19 Nov 2018 15:58:00 +0200	[thread overview]
Message-ID: <20181119135800.GE8755@linux.intel.com> (raw)
In-Reply-To: <90072f09-6388-b1f7-387b-42f056f8893d@intel.com>

On Sun, Nov 18, 2018 at 07:05:19PM -0800, Tadeusz Struk wrote:
> > When ret_size < 0? Shouldn't this be just "if (!ret_size)"?
> 
> What we want to check here is if ret_size is positive, which is a valid
> value, or if it is negative or zero, which is an invalid value, so in
> this case (!ret_size) will not work.

Please explain a scenario where "!ret_size" would no work given that
both size and partial_data have always positive value?

> >> 	/* Holds the resul of the last successful call to tpm_transmit() */
> >>  	size_t transmit_result;
> >> +	/* Holds the count how much of the response is still unread */
> >> +	size_t partial_data;
> > I'm otherwise happy how this look like but why call it partial_data.
> > You cannot really tell from the name anything about its contents as
> > data is very abstract term.
>  
> so I will rename these two to response_length and response_length_rem,
> how does this sound?

Yes, assuming that there would be a hard requirement to even have two
variables in the first place.

> > BTW, why you need the new variable anyway and not just decrease the
> > variable where the length is original stored?
> 
> We need to have two variables, otherwise how do we tell if some part of
> response was consumed to allow sending a new command?

I don't understand. In order to maintain backwards compatibility you can
send a new command at any time.

> The transmit_result is used for that. If it is zero then one can transmit
> a new command even if the whole response is not consumed. The new variable
> tracks how much of the response is still to be read. 

AFAIK you only need to track the latter, not both.

/Jarkko