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);
+ 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
--
Anthony PERARD
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47215): https://edk2.groups.io/g/devel/message/47215
Mute This Topic: https://groups.io/mt/34128016/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47284): https://edk2.groups.io/g/devel/message/47284
Mute This Topic: https://groups.io/mt/34128016/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47286): https://edk2.groups.io/g/devel/message/47286
Mute This Topic: https://groups.io/mt/34128016/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.