Re: [edk2-devel] [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions
On 09/13/19 16:50, Anthony PERARD wrote:
> We will use a buffer on the stack instead of allocating memory for
> internal functions that are expecting a reply from xenstore.
>
> The external interface XENBUS_PROTOCOL isn't changed yet, so
> allocation are made for XsRead and XsBackendRead.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD
> ---
> OvmfPkg/XenBusDxe/XenBus.c | 40 ++--
> OvmfPkg/XenBusDxe/XenStore.c | 115 ---
> OvmfPkg/XenBusDxe/XenStore.h | 17 +++---
> 3 files changed, 95 insertions(+), 77 deletions(-)
>
Quoting out of order:
> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> index effaad7336..13f7d132e6 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.h
> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> @@ -64,29 +64,26 @@ XenStorePathExists (
>);
>
> /**
> - Get the contents of a single "file". Returns the contents in *Result which
> - should be freed after use. The length of the value in bytes is returned in
> - *LenPtr.
> + Get the contents of a single "file". Copy the contents in Buffer if
> + provided. The length of the value in bytes is returned in *BufferSize.
>
>@param TransactionThe XenStore transaction covering this request.
>@param DirectoryPath The dirname of the file to read.
>@param Node The basename of the file to read.
> - @param LenPtr The amount of data read.
> - @param Result The returned contents from this file.
> + @param BufferSize IN: size of the buffer
> +OUT: The returned length of the reply.
> + @param Buffer The returned body of the reply.
>
>@return On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
> indicating the type of failure.
(1) I assume this -- i.e. error returned -- covers the case when
BufferSize was too small on input. Can you document that? (Or should it
be obvious from elsewhere?)
> -
> - @note The results buffer is malloced and should be free'd by the
> -caller.
> **/
> XENSTORE_STATUS
> XenStoreRead (
>IN CONST XENSTORE_TRANSACTION *Transaction,
>IN CONST CHAR8 *DirectoryPath,
>IN CONST CHAR8 *Node,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID**Result
> + IN OUT UINTN*BufferSize,
> + OUT VOID*Buffer
>);
>
> /**
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 004d3b6022..b9588bb8c6 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -756,8 +756,9 @@ XenStoreGetError (
>@param RequestTypeThe type of message to send.
>@param WriteRequest Pointers to the body sections of the request.
>@param NumRequestsThe number of body sections in the request.
> - @param LenPtr The returned length of the reply.
> - @param ResultPtr The returned body of the reply.
> + @param BufferSize IN: size of the buffer
> +OUT: The returned length of the reply.
> + @param Buffer The returned body of the reply.
>
>@return XENSTORE_STATUS_SUCCESS on success. Otherwise an errno indicating
> the cause of failure.
> @@ -769,15 +770,13 @@ XenStoreTalkv (
>IN enum xsd_sockmsg_type RequestType,
>IN CONST WRITE_REQUEST *WriteRequest,
>IN UINT32 NumRequests,
> - OUT UINT32 *LenPtr OPTIONAL,
> - OUT VOID**ResultPtr OPTIONAL
> + IN OUT UINTN*BufferSize OPTIONAL,
> + OUT VOID*Buffer OPTIONAL
>)
> {
>struct xsd_sockmsg Message;
>UINTN Index;
>XENSTORE_STATUSStatus;
> - VOID *Buffer;
> - UINTN BufferSize;
>
>if (Transaction == XST_NIL) {
> Message.tx_id = 0;
> @@ -805,32 +804,15 @@ XenStoreTalkv (
> }
>}
>
> - if (ResultPtr) {
> -Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
> -BufferSize = XENSTORE_PAYLOAD_MAX;
> - } else {
> -Buffer = NULL;
> -BufferSize = 0;
> - }
> -
>//
>// Wait for a reply to our request
>//
>Status = XenStoreProcessMessage (Message.req_id, Message.tx_id,
> -, Buffer);
> +BufferSize, Buffer);
(2) Since we're touching this -- please fix the indentation.
>
>if (Status != XENSTORE_STATUS_SUCCESS) {
> DEBUG ((DEBUG_ERROR, "XenStore, error while reading the ring (%d).\n",
> Status));
> -FreePool (Buffer);
> -return Status;
> - }
> -
> - if (ResultPtr) {
> -*ResultPtr = Buffer;
> -if (LenPtr) {
> - *LenPtr = BufferSize;
> -}
>}
>
> Error:
> @@ -848,8 +830,9 @@ XenStoreTalkv (
>@param RequestTypeThe type of message to send.
>@param Body The body of the request.
>@param SubPathIf !NULL and not "", "/$SubPath" is append to Body.
> - @param
[edk2-devel] [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions
We will use a buffer on the stack instead of allocating memory for
internal functions that are expecting a reply from xenstore.
The external interface XENBUS_PROTOCOL isn't changed yet, so
allocation are made for XsRead and XsBackendRead.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
Signed-off-by: Anthony PERARD
---
OvmfPkg/XenBusDxe/XenBus.c | 40 ++--
OvmfPkg/XenBusDxe/XenStore.c | 115 ---
OvmfPkg/XenBusDxe/XenStore.h | 17 +++---
3 files changed, 95 insertions(+), 77 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
index bb8ddbc4d4..78835ec7b3 100644
--- a/OvmfPkg/XenBusDxe/XenBus.c
+++ b/OvmfPkg/XenBusDxe/XenBus.c
@@ -89,19 +89,18 @@ XenBusReadDriverState (
IN CONST CHAR8 *Path
)
{
- XenbusState State;
- CHAR8 *Ptr = NULL;
+ XenbusState State;
+ CHAR8 Buffer[4];
+ UINTN BufferSize;
XENSTORE_STATUS Status;
- Status = XenStoreRead (XST_NIL, Path, "state", NULL, (VOID **));
+ BufferSize = sizeof (Buffer) - 1;
+ Status = XenStoreRead (XST_NIL, Path, "state", , Buffer);
if (Status != XENSTORE_STATUS_SUCCESS) {
State = XenbusStateClosed;
} else {
-State = AsciiStrDecimalToUintn (Ptr);
- }
-
- if (Ptr != NULL) {
-FreePool (Ptr);
+Buffer[BufferSize] = '\0';
+State = AsciiStrDecimalToUintn (Buffer);
}
return State;
@@ -129,8 +128,11 @@ XenBusAddDevice (
if (XenStorePathExists (XST_NIL, DevicePath, "")) {
XENBUS_PRIVATE_DATA *Child;
-enum xenbus_state State;
-CHAR8 *BackendPath;
+enum xenbus_state State;
+CHAR8 BackendPath[XENSTORE_ABS_PATH_MAX + 1];
+UINTN BackendPathSize;
+
+BackendPathSize = sizeof (BackendPath);
Child = XenBusDeviceInitialized (Dev, DevicePath);
if (Child != NULL) {
@@ -155,17 +157,18 @@ XenBusAddDevice (
}
StatusXenStore = XenStoreRead (XST_NIL, DevicePath, "backend",
- NULL, (VOID **) );
+ , BackendPath);
if (StatusXenStore != XENSTORE_STATUS_SUCCESS) {
DEBUG ((EFI_D_ERROR, "xenbus: %a no backend path.\n", DevicePath));
Status = EFI_NOT_FOUND;
goto out;
}
+BackendPath[BackendPathSize] = '\0';
Private = AllocateCopyPool (sizeof (*Private), );
Private->XenBusIo.Type = AsciiStrDup (Type);
Private->XenBusIo.Node = AsciiStrDup (DevicePath);
-Private->XenBusIo.Backend = BackendPath;
+Private->XenBusIo.Backend = AsciiStrDup (BackendPath);
Private->XenBusIo.DeviceId = (UINT16)AsciiStrDecimalToUintn (Id);
Private->Dev = Dev;
@@ -309,17 +312,20 @@ XenBusSetState (
)
{
enum xenbus_state CurrentState;
- XENSTORE_STATUS Status;
- CHAR8 *Temp;
+ XENSTORE_STATUS Status;
+ CHAR8 Buffer[4];
+ UINTN BufferSize;
+
+ BufferSize = sizeof (Buffer) - 1;
DEBUG ((EFI_D_INFO, "XenBus: Set state to %d\n", NewState));
- Status = XenStoreRead (Transaction, This->Node, "state", NULL, (VOID
**));
+ Status = XenStoreRead (Transaction, This->Node, "state", ,
Buffer);
if (Status != XENSTORE_STATUS_SUCCESS) {
goto Out;
}
- CurrentState = AsciiStrDecimalToUintn (Temp);
- FreePool (Temp);
+ Buffer[BufferSize] = '\0';
+ CurrentState = AsciiStrDecimalToUintn (Buffer);
if (CurrentState == NewState) {
goto Out;
}
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 004d3b6022..b9588bb8c6 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -756,8 +756,9 @@ XenStoreGetError (
@param RequestTypeThe type of message to send.
@param WriteRequest Pointers to the body sections of the request.
@param NumRequestsThe number of body sections in the request.
- @param LenPtr The returned length of the reply.
- @param ResultPtr The returned body of the reply.
+ @param BufferSize IN: size of the buffer
+OUT: The returned length of the reply.
+ @param Buffer The returned body of the reply.
@return XENSTORE_STATUS_SUCCESS on success. Otherwise an errno indicating
the cause of failure.
@@ -769,15 +770,13 @@ XenStoreTalkv (
IN enum xsd_sockmsg_type RequestType,
IN CONST WRITE_REQUEST *WriteRequest,
IN UINT32 NumRequests,
- OUT UINT32 *LenPtr OPTIONAL,
- OUT VOID**ResultPtr OPTIONAL
+ IN OUT UINTN*BufferSize OPTIONAL,
+ OUT VOID*Buffer OPTIONAL
)
{
struct xsd_sockmsg Message;
UINTN Index;
XENSTORE_STATUSStatus;
- VOID *Buffer;
- UINTN BufferSize;
if (Transaction == XST_NIL) {
Message.tx_id = 0;
@@ -805,32 +804,15 @@ XenStoreTalkv (
}
}
- if (ResultPtr) {
-Buffer = AllocatePool (XENSTORE_PAYLOAD_MAX + 1);
-BufferSize = XENSTORE_PAYLOAD_MAX;
- } else {
-Buffer =
