Re: [PATCH v2 09/16] block: sed-opal: split generation of bytestring header and content

Re: [PATCH v2 09/16] block: sed-opal: split generation of bytestring header and content

[Scott Bauer]

On Thu, Jan 17, 2019 at 09:31:49PM +0000, David Kozub wrote:
> -
> -	memcpy(&cmd->cmd[cmd->pos], bytestring, len);
> +	start = &cmd->cmd[cmd->pos];
>  	cmd->pos += len;

This is somewhat pendatic, but it helps me review patches if we keep things together.
Since we're no longer doing the memcpy in this function, can we please move the cmd->pos += len
to the location where we actually do the memcpy.

I'm willing to be told to get over it if other reviewers don't like that approach, but if no one cares
please move it.


> +	return start;
> +}
>  
> +static void add_token_bytestring(int *err, struct opal_dev *cmd,
> +				 const u8 *bytestring, size_t len)
> +{
> +	u8 *start;
> +
> +	start = add_bytestring_header(err, cmd, len);
> +	if (!start)
> +		return;
> +	memcpy(start, bytestring, len);

Do the above here instead.

> 2.20.1
> 
> 

[PATCH v2 09/16] block: sed-opal: split generation of bytestring header and content

[David Kozub]

From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>

Split the header generation from the (normal) memcpy part if a
bytestring is copied into the command buffer. This allows in-place
generation of the bytestring content. For example, copy_from_user may be
used without an intermediate buffer.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
---
 block/sed-opal.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 4225f23b2165..db966bbc5a05 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -586,14 +586,11 @@ static void add_token_u64(int *err, struct opal_dev *cmd, u64 number)
 		add_token_u8(err, cmd, number >> (len * 8));
 }
 
-static void add_token_bytestring(int *err, struct opal_dev *cmd,
-				 const u8 *bytestring, size_t len)
+static u8 *add_bytestring_header(int *err, struct opal_dev *cmd, size_t len)
 {
 	size_t header_len = 1;
 	bool is_short_atom = true;
-
-	if (*err)
-		return;
+	char *start;
 
 	if (len & ~SHORT_ATOM_LEN_MASK) {
 		header_len = 2;
@@ -602,17 +599,27 @@ static void add_token_bytestring(int *err, struct opal_dev *cmd,
 
 	if (!can_add(err, cmd, header_len + len)) {
 		pr_debug("Error adding bytestring: end of buffer.\n");
-		return;
+		return NULL;
 	}
 
 	if (is_short_atom)
 		add_short_atom_header(cmd, true, false, len);
 	else
 		add_medium_atom_header(cmd, true, false, len);
-
-	memcpy(&cmd->cmd[cmd->pos], bytestring, len);
+	start = &cmd->cmd[cmd->pos];
 	cmd->pos += len;
+	return start;
+}
 
+static void add_token_bytestring(int *err, struct opal_dev *cmd,
+				 const u8 *bytestring, size_t len)
+{
+	u8 *start;
+
+	start = add_bytestring_header(err, cmd, len);
+	if (!start)
+		return;
+	memcpy(start, bytestring, len);
 }
 
 static int build_locking_range(u8 *buffer, size_t length, u8 lr)
-- 
2.20.1