[05/11] OvmfPkg/XenBusDxe: Construct paths without allocation
diff mbox series

Message ID 20190913145100.303433-6-anthony.perard@citrix.com
State New
Headers show
Series
  • OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation
Related show

Commit Message

Anthony PERARD Sept. 13, 2019, 2:50 p.m. UTC
When doing an action with a path and subpath in the xenstore,
XenStoreJoin is called to generate "$path/$subpath". But this function
do an allocation of memory which isn't necessary. Instead we will
construct the path with WRITE_REQUEST and data used to generate the
path will be copied directly to the xenstore shared ring.

Also change WRITE_REQUEST.Len type, it only contain sizes and doesn't
need to be exactly 32bits.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 OvmfPkg/XenBusDxe/XenStore.c | 78 +++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 32 deletions(-)

Comments

Laszlo Ersek Sept. 16, 2019, 3:39 p.m. UTC | #1
On 09/13/19 16:50, Anthony PERARD wrote:
> When doing an action with a path and subpath in the xenstore,
> XenStoreJoin is called to generate "$path/$subpath". But this function
> do an allocation of memory which isn't necessary. Instead we will
> construct the path with WRITE_REQUEST and data used to generate the
> path will be copied directly to the xenstore shared ring.
> 
> Also change WRITE_REQUEST.Len type, it only contain sizes and doesn't
> need to be exactly 32bits.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenBusDxe/XenStore.c | 78 +++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 32 deletions(-)
> 
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 7b71dc156d..ca7be12d68 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -53,7 +53,7 @@
>  
>  typedef struct {
>    CONST VOID  *Data;
> -  UINT32      Len;
> +  UINTN       Len;
>  } WRITE_REQUEST;
>  
>  /* Register callback to watch subtree (node) in the XenStore. */
> @@ -260,6 +260,35 @@ XenStoreFindWatch (
>    return NULL;
>  }
>  
> +/**
> +  Fill the first three slots of a WRITE_REQUEST array.
> +
> +  When those three slots are concatenated to generate a string, the resulting
> +  string will be "$Path\0" or "$Path/$SubPath\0" if SubPath is provided.
> +**/
> +STATIC
> +VOID
> +XenStorePrepareWriteRequest (
> +  IN OUT WRITE_REQUEST *WriteRequest,
> +  IN     CONST CHAR8   *Path,
> +  IN     CONST CHAR8   *SubPath OPTIONAL
> +  )
> +{
> +  SetMem(WriteRequest, 3 * sizeof (WRITE_REQUEST), 0);

(1) ZeroMem() is more idiomatic. Also, please insert a space before the
opening paren.

> +  WriteRequest[0].Data = Path;
> +  WriteRequest[0].Len = AsciiStrSize (Path);
> +  if (SubPath != NULL && SubPath[0] != '\0') {
> +    //
> +    // Remove the \0 from the first part of the request.
> +    //
> +    WriteRequest[0].Len--;
> +    WriteRequest[1].Data = "/";
> +    WriteRequest[1].Len = 1;
> +    WriteRequest[2].Data = SubPath;
> +    WriteRequest[2].Len = AsciiStrSize (SubPath);
> +  }
> +}
> +

So this suggests that only the last element in the array should point to
a NUL-terminated string. Strings pointed-to by earlier elements in the
array should not be NUL-terminated. Is that correct?

>  //
>  // Public Utility Functions
>  // API comments for these methods can be found in XenStore.h
> @@ -842,6 +871,7 @@ XenStoreTalkv (
>    @param Transaction    The transaction to use for this request.
>    @param RequestType    The type of message to send.
>    @param Body           The body of the request.
> +  @param SubPath        If !NULL and not "", "/$SubPath" is append to Body.
>    @param LenPtr         The returned length of the reply.
>    @param Result         The returned body of the reply.
>  
> @@ -854,16 +884,16 @@ XenStoreSingle (
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  enum xsd_sockmsg_type   RequestType,
>    IN  CONST CHAR8             *Body,
> +  IN  CONST CHAR8             *SubPath OPTIONAL,
>    OUT UINT32                  *LenPtr OPTIONAL,
>    OUT VOID                    **Result OPTIONAL
>    )
>  {
> -  WRITE_REQUEST WriteRequest;
> +  WRITE_REQUEST   WriteRequest[3];
>  
> -  WriteRequest.Data = (VOID *) Body;
> -  WriteRequest.Len = (UINT32)AsciiStrSize (Body);
> +  XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
>  
> -  return XenStoreTalkv (Transaction, RequestType, &WriteRequest, 1,
> +  return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
>                          LenPtr, Result);

(2) It would be slightly more idiomatic to pass

  ARRAY_SIZE (WriteRequest)

in place of the naked 3.

>  }
>  
> @@ -1113,15 +1143,12 @@ XenStoreListDirectory (
>    OUT CONST CHAR8           ***DirectoryListPtr
>    )
>  {
> -  CHAR8 *Path;
>    CHAR8 *TempStr;
>    UINT32 Len = 0;
>    XENSTORE_STATUS Status;
>  
> -  Path = XenStoreJoin (DirectoryPath, Node);
> -  Status = XenStoreSingle (Transaction, XS_DIRECTORY, Path, &Len,
> +  Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
>                             (VOID **) &TempStr);
> -  FreePool (Path);
>    if (Status != XENSTORE_STATUS_SUCCESS) {
>      return Status;
>    }
> @@ -1160,13 +1187,11 @@ XenStoreRead (
>    OUT VOID                    **Result
>    )
>  {
> -  CHAR8 *Path;
>    VOID *Value;
>    XENSTORE_STATUS Status;
>  
> -  Path = XenStoreJoin (DirectoryPath, Node);
> -  Status = XenStoreSingle (Transaction, XS_READ, Path, LenPtr, &Value);
> -  FreePool (Path);
> +  Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
> +    LenPtr, &Value);

(3) Indentation.

>    if (Status != XENSTORE_STATUS_SUCCESS) {
>      return Status;
>    }
> @@ -1183,21 +1208,13 @@ XenStoreWrite (
>    IN CONST CHAR8           *Str
>    )
>  {
> -  CHAR8 *Path;
> -  WRITE_REQUEST WriteRequest[2];
> -  XENSTORE_STATUS Status;
> +  WRITE_REQUEST   WriteRequest[4];
>  
> -  Path = XenStoreJoin (DirectoryPath, Node);
> +  XenStorePrepareWriteRequest (WriteRequest, DirectoryPath, Node);
> +  WriteRequest[3].Data = Str;
> +  WriteRequest[3].Len = AsciiStrLen (Str);

Now we have two strings, pointed-to by elements in the array, that are
NUL-terminated: the element at offset 2, and the one at offset 3. Is
that intentional? Is that part of the message framing?

Hmmm... From the original code:

>  
> -  WriteRequest[0].Data = (VOID *) Path;
> -  WriteRequest[0].Len = (UINT32)AsciiStrSize (Path);
> -  WriteRequest[1].Data = (VOID *) Str;
> -  WriteRequest[1].Len = (UINT32)AsciiStrLen (Str);

That seems to be the case. I guess the first run (offsets 0 through 2)
is parsed until the first NUL is encountered, for "path", then the
second run (3 and onwards) is parsed until the second NUL for "data".
Sounds plausible; OK.

> -
> -  Status = XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 2, NULL, NULL);
> -  FreePool (Path);
> -
> -  return Status;
> +  return XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 4, NULL, NULL);

(4) Please use ARRAY_SIZE(); it's more robust.

>  }
>  
>  XENSTORE_STATUS
> @@ -1207,12 +1224,9 @@ XenStoreRemove (
>    IN CONST CHAR8            *Node
>    )
>  {
> -  CHAR8 *Path;
>    XENSTORE_STATUS Status;
>  
> -  Path = XenStoreJoin (DirectoryPath, Node);
> -  Status = XenStoreSingle (Transaction, XS_RM, Path, NULL, NULL);
> -  FreePool (Path);
> +  Status = XenStoreSingle (Transaction, XS_RM, DirectoryPath, Node, NULL, NULL);
>  
>    return Status;
>  }
> @@ -1226,7 +1240,7 @@ XenStoreTransactionStart (
>    XENSTORE_STATUS Status;
>  
>    Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
> -                           (VOID **) &IdStr);
> +    NULL, (VOID **) &IdStr);

(5) Indentation.

>    if (Status == XENSTORE_STATUS_SUCCESS) {
>      Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
>      FreePool (IdStr);
> @@ -1246,7 +1260,7 @@ XenStoreTransactionEnd (
>    AbortStr[0] = Abort ? 'F' : 'T';
>    AbortStr[1] = '\0';
>  
> -  return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
> +  return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL, NULL);
>  }
>  
>  XENSTORE_STATUS
> 

With the above addressed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Laszlo Ersek Sept. 16, 2019, 3:43 p.m. UTC | #2
On 09/16/19 17:39, Laszlo Ersek wrote:
> On 09/13/19 16:50, Anthony PERARD wrote:
>> When doing an action with a path and subpath in the xenstore,
>> XenStoreJoin is called to generate "$path/$subpath". But this function
>> do an allocation of memory which isn't necessary. Instead we will
>> construct the path with WRITE_REQUEST and data used to generate the
>> path will be copied directly to the xenstore shared ring.
>>
>> Also change WRITE_REQUEST.Len type, it only contain sizes and doesn't
>> need to be exactly 32bits.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  OvmfPkg/XenBusDxe/XenStore.c | 78 +++++++++++++++++++++---------------
>>  1 file changed, 46 insertions(+), 32 deletions(-)
>>
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
>> index 7b71dc156d..ca7be12d68 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>> @@ -53,7 +53,7 @@
>>  
>>  typedef struct {
>>    CONST VOID  *Data;
>> -  UINT32      Len;
>> +  UINTN       Len;
>>  } WRITE_REQUEST;
>>  
>>  /* Register callback to watch subtree (node) in the XenStore. */
>> @@ -260,6 +260,35 @@ XenStoreFindWatch (
>>    return NULL;
>>  }
>>  
>> +/**
>> +  Fill the first three slots of a WRITE_REQUEST array.
>> +
>> +  When those three slots are concatenated to generate a string, the resulting
>> +  string will be "$Path\0" or "$Path/$SubPath\0" if SubPath is provided.
>> +**/
>> +STATIC
>> +VOID
>> +XenStorePrepareWriteRequest (
>> +  IN OUT WRITE_REQUEST *WriteRequest,

(6) I think this could be just OUT -- we start by zeroing it out.

>> +  IN     CONST CHAR8   *Path,
>> +  IN     CONST CHAR8   *SubPath OPTIONAL
>> +  )
>> +{
>> +  SetMem(WriteRequest, 3 * sizeof (WRITE_REQUEST), 0);

Thanks
Laszlo

Patch
diff mbox series

diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 7b71dc156d..ca7be12d68 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -53,7 +53,7 @@ 
 
 typedef struct {
   CONST VOID  *Data;
-  UINT32      Len;
+  UINTN       Len;
 } WRITE_REQUEST;
 
 /* Register callback to watch subtree (node) in the XenStore. */
@@ -260,6 +260,35 @@  XenStoreFindWatch (
   return NULL;
 }
 
+/**
+  Fill the first three slots of a WRITE_REQUEST array.
+
+  When those three slots are concatenated to generate a string, the resulting
+  string will be "$Path\0" or "$Path/$SubPath\0" if SubPath is provided.
+**/
+STATIC
+VOID
+XenStorePrepareWriteRequest (
+  IN OUT WRITE_REQUEST *WriteRequest,
+  IN     CONST CHAR8   *Path,
+  IN     CONST CHAR8   *SubPath OPTIONAL
+  )
+{
+  SetMem(WriteRequest, 3 * sizeof (WRITE_REQUEST), 0);
+  WriteRequest[0].Data = Path;
+  WriteRequest[0].Len = AsciiStrSize (Path);
+  if (SubPath != NULL && SubPath[0] != '\0') {
+    //
+    // Remove the \0 from the first part of the request.
+    //
+    WriteRequest[0].Len--;
+    WriteRequest[1].Data = "/";
+    WriteRequest[1].Len = 1;
+    WriteRequest[2].Data = SubPath;
+    WriteRequest[2].Len = AsciiStrSize (SubPath);
+  }
+}
+
 //
 // Public Utility Functions
 // API comments for these methods can be found in XenStore.h
@@ -842,6 +871,7 @@  XenStoreTalkv (
   @param Transaction    The transaction to use for this request.
   @param RequestType    The type of message to send.
   @param Body           The body of the request.
+  @param SubPath        If !NULL and not "", "/$SubPath" is append to Body.
   @param LenPtr         The returned length of the reply.
   @param Result         The returned body of the reply.
 
@@ -854,16 +884,16 @@  XenStoreSingle (
   IN  CONST XENSTORE_TRANSACTION *Transaction,
   IN  enum xsd_sockmsg_type   RequestType,
   IN  CONST CHAR8             *Body,
+  IN  CONST CHAR8             *SubPath OPTIONAL,
   OUT UINT32                  *LenPtr OPTIONAL,
   OUT VOID                    **Result OPTIONAL
   )
 {
-  WRITE_REQUEST WriteRequest;
+  WRITE_REQUEST   WriteRequest[3];
 
-  WriteRequest.Data = (VOID *) Body;
-  WriteRequest.Len = (UINT32)AsciiStrSize (Body);
+  XenStorePrepareWriteRequest (WriteRequest, Body, SubPath);
 
-  return XenStoreTalkv (Transaction, RequestType, &WriteRequest, 1,
+  return XenStoreTalkv (Transaction, RequestType, WriteRequest, 3,
                         LenPtr, Result);
 }
 
@@ -1113,15 +1143,12 @@  XenStoreListDirectory (
   OUT CONST CHAR8           ***DirectoryListPtr
   )
 {
-  CHAR8 *Path;
   CHAR8 *TempStr;
   UINT32 Len = 0;
   XENSTORE_STATUS Status;
 
-  Path = XenStoreJoin (DirectoryPath, Node);
-  Status = XenStoreSingle (Transaction, XS_DIRECTORY, Path, &Len,
+  Status = XenStoreSingle (Transaction, XS_DIRECTORY, DirectoryPath, Node, &Len,
                            (VOID **) &TempStr);
-  FreePool (Path);
   if (Status != XENSTORE_STATUS_SUCCESS) {
     return Status;
   }
@@ -1160,13 +1187,11 @@  XenStoreRead (
   OUT VOID                    **Result
   )
 {
-  CHAR8 *Path;
   VOID *Value;
   XENSTORE_STATUS Status;
 
-  Path = XenStoreJoin (DirectoryPath, Node);
-  Status = XenStoreSingle (Transaction, XS_READ, Path, LenPtr, &Value);
-  FreePool (Path);
+  Status = XenStoreSingle (Transaction, XS_READ, DirectoryPath, Node,
+    LenPtr, &Value);
   if (Status != XENSTORE_STATUS_SUCCESS) {
     return Status;
   }
@@ -1183,21 +1208,13 @@  XenStoreWrite (
   IN CONST CHAR8           *Str
   )
 {
-  CHAR8 *Path;
-  WRITE_REQUEST WriteRequest[2];
-  XENSTORE_STATUS Status;
+  WRITE_REQUEST   WriteRequest[4];
 
-  Path = XenStoreJoin (DirectoryPath, Node);
+  XenStorePrepareWriteRequest (WriteRequest, DirectoryPath, Node);
+  WriteRequest[3].Data = Str;
+  WriteRequest[3].Len = AsciiStrLen (Str);
 
-  WriteRequest[0].Data = (VOID *) Path;
-  WriteRequest[0].Len = (UINT32)AsciiStrSize (Path);
-  WriteRequest[1].Data = (VOID *) Str;
-  WriteRequest[1].Len = (UINT32)AsciiStrLen (Str);
-
-  Status = XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 2, NULL, NULL);
-  FreePool (Path);
-
-  return Status;
+  return XenStoreTalkv (Transaction, XS_WRITE, WriteRequest, 4, NULL, NULL);
 }
 
 XENSTORE_STATUS
@@ -1207,12 +1224,9 @@  XenStoreRemove (
   IN CONST CHAR8            *Node
   )
 {
-  CHAR8 *Path;
   XENSTORE_STATUS Status;
 
-  Path = XenStoreJoin (DirectoryPath, Node);
-  Status = XenStoreSingle (Transaction, XS_RM, Path, NULL, NULL);
-  FreePool (Path);
+  Status = XenStoreSingle (Transaction, XS_RM, DirectoryPath, Node, NULL, NULL);
 
   return Status;
 }
@@ -1226,7 +1240,7 @@  XenStoreTransactionStart (
   XENSTORE_STATUS Status;
 
   Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
-                           (VOID **) &IdStr);
+    NULL, (VOID **) &IdStr);
   if (Status == XENSTORE_STATUS_SUCCESS) {
     Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
     FreePool (IdStr);
@@ -1246,7 +1260,7 @@  XenStoreTransactionEnd (
   AbortStr[0] = Abort ? 'F' : 'T';
   AbortStr[1] = '\0';
 
-  return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
+  return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL, NULL);
 }
 
 XENSTORE_STATUS