comments below
On 11/13/14 18:27, Anthony PERARD wrote:
> From: Scott Duplichan <[email protected]>
>
> This patch contain only type cast.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Scott Duplichan <[email protected]>
> Signed-off-by: Anthony PERARD <[email protected]>
> ---
> OvmfPkg/XenBusDxe/EventChannel.c | 6 +++---
> OvmfPkg/XenBusDxe/GrantTable.c | 4 ++--
> OvmfPkg/XenBusDxe/XenBus.c | 6 +++---
> OvmfPkg/XenBusDxe/XenHypercall.c | 6 +++---
> OvmfPkg/XenBusDxe/XenStore.c | 22 +++++++++++-----------
> 5 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/EventChannel.c
> b/OvmfPkg/XenBusDxe/EventChannel.c
> index 547c111..03efaf9 100644
> --- a/OvmfPkg/XenBusDxe/EventChannel.c
> +++ b/OvmfPkg/XenBusDxe/EventChannel.c
> @@ -29,7 +29,7 @@ XenEventChannelNotify (
>
> Send.port = Port;
> ReturnCode = XenHypercallEventChannelOp (Dev, EVTCHNOP_send, &Send);
> - return ReturnCode;
> + return (UINT32)ReturnCode;
> }
Not sure why it is safe to cast (without loss of information) an INTN
retval to UINT32, on X64.
>
> UINT32
> @@ -48,7 +48,7 @@ XenBusEventChannelAllocate (
>
> Parameter.dom = DOMID_SELF;
> Parameter.remote_dom = DomainId;
> - ReturnCode = XenHypercallEventChannelOp (Private->Dev,
> + ReturnCode = (UINT32)XenHypercallEventChannelOp (Private->Dev,
> EVTCHNOP_alloc_unbound,
> &Parameter);
> if (ReturnCode != 0) {
ditto
> @@ -84,5 +84,5 @@ XenBusEventChannelClose (
>
> Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
> Close.port = Port;
> - return XenHypercallEventChannelOp (Private->Dev, EVTCHNOP_close, &Close);
> + return (UINT32)XenHypercallEventChannelOp (Private->Dev, EVTCHNOP_close,
> &Close);
> }
ditto
> diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
> index f68a854..37d3bf7 100644
> --- a/OvmfPkg/XenBusDxe/GrantTable.c
> +++ b/OvmfPkg/XenBusDxe/GrantTable.c
> @@ -101,7 +101,7 @@ XenGrantTableGrantAccess (
>
> ASSERT (GrantTable != NULL);
> Ref = XenGrantTableGetFreeEntry ();
> - GrantTable[Ref].frame = Frame;
> + GrantTable[Ref].frame = (UINT32)Frame;
> GrantTable[Ref].domid = DomainId;
> MemoryFence ();
> Flags = GTF_permit_access;
> @@ -152,7 +152,7 @@ XenGrantTableInit (
> #endif
> EfiInitializeLock (&mGrantListLock, TPL_NOTIFY);
> for (Index = NR_RESERVED_ENTRIES; Index < NR_GRANT_ENTRIES; Index++) {
> - XenGrantTablePutFreeEntry (Index);
> + XenGrantTablePutFreeEntry ((grant_ref_t)Index);
> }
>
> GrantTable = (VOID*)(UINTN) MmioAddr;
> diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
> index a735058..f69c27d 100644
> --- a/OvmfPkg/XenBusDxe/XenBus.c
> +++ b/OvmfPkg/XenBusDxe/XenBus.c
> @@ -182,7 +182,7 @@ XenBusAddDevice (
> Private->XenBusIo.Type = AsciiStrDup (Type);
> Private->XenBusIo.Node = AsciiStrDup (DevicePath);
> Private->XenBusIo.Backend = BackendPath;
> - Private->XenBusIo.DeviceId = AsciiStrDecimalToUintn (Id);
> + Private->XenBusIo.DeviceId = (UINT16)AsciiStrDecimalToUintn (Id);
> Private->Dev = Dev;
>
> TempXenBusPath = AllocateCopyPool (sizeof (XENBUS_DEVICE_PATH),
> @@ -274,7 +274,7 @@ XenBusEnumerateDeviceType (
> XenBusAddDevice (Dev, Type, Directory[Index]);
> }
>
> - FreePool (Directory);
> + FreePool ((VOID*)Directory);
> }
>
>
> @@ -310,7 +310,7 @@ XenBusEnumerateBus (
> XenBusEnumerateDeviceType (Dev, Types[Index]);
> }
>
> - FreePool (Types);
> + FreePool ((VOID*)Types);
>
> return XENSTORE_STATUS_SUCCESS;
> }
> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c
> b/OvmfPkg/XenBusDxe/XenHypercall.c
> index 15e9e1c..34d92e7 100644
> --- a/OvmfPkg/XenBusDxe/XenHypercall.c
> +++ b/OvmfPkg/XenBusDxe/XenHypercall.c
> @@ -53,7 +53,7 @@ XenHypercallHvmGetParam (
>
> Parameter.domid = DOMID_SELF;
> Parameter.index = Index;
> - Error = XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_hvm_op * 32,
> + Error = XenHypercall2 ((UINT8*)Dev->Hyperpage + __HYPERVISOR_hvm_op * 32,
> HVMOP_get_param, (INTN) &Parameter);
> if (Error != 0) {
> DEBUG ((EFI_D_ERROR,
> @@ -72,7 +72,7 @@ XenHypercallMemoryOp (
> )
> {
> ASSERT (Dev->Hyperpage != NULL);
> - return XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_memory_op * 32,
> + return XenHypercall2 ((UINT8*)Dev->Hyperpage + __HYPERVISOR_memory_op * 32,
> Operation, (INTN) Arguments);
> }
>
> @@ -84,7 +84,7 @@ XenHypercallEventChannelOp (
> )
> {
> ASSERT (Dev->Hyperpage != NULL);
> - return XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_event_channel_op * 32,
> + return XenHypercall2 ((UINT8*)Dev->Hyperpage +
> __HYPERVISOR_event_channel_op * 32,
> Operation, (INTN) Arguments);
> }
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 7c272b3..2df8f53 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -248,7 +248,7 @@ Split (
>
> /* Transfer to one big alloc for easy freeing by the caller. */
> Dst = AllocatePool (*NumPtr * sizeof (CHAR8 *) + Len);
> - CopyMem (&Dst[*NumPtr], Strings, Len);
> + CopyMem ((VOID*)&Dst[*NumPtr], Strings, Len);
This looks very much like undefined behavior in standard C, but the
patch is independent of that.
> FreePool (Strings);
>
> /* Extract pointers to newly allocated array. */
> @@ -660,7 +660,7 @@ XenStoreProcessMessage (
> } else {
> DEBUG ((EFI_D_WARN, "XenStore: Watch handle %a not found\n",
> Message->u.Watch.Vector[XS_WATCH_TOKEN]));
> - FreePool(Message->u.Watch.Vector);
> + FreePool((VOID*)Message->u.Watch.Vector);
> FreePool(Message);
> }
> EfiReleaseLock (&xs.RegisteredWatchesLock);
> @@ -829,7 +829,7 @@ XenStoreTalkv (
> }
> }
>
> - Status = XenStoreReadReply (&Message.type, LenPtr, &Return);
> + Status = XenStoreReadReply ((enum xsd_sockmsg_type *)&Message.type,
> LenPtr, &Return);
This seems wrong, generally speaking (again, independently of the patch).
Namely, while enumeration constants are required by the C standard to
have type "int" (which corresponds to INT32 in edk2), the enumerated
type itself
shall be compatible with char, a signed integer type, or an
unsigned integer type. The choice of type is implementation-
defined, but shall be capable of representing the values of all the
members of the enumeration.
Looking at "enum xsd_sockmsg_type", a conformant platform might
perfectly well could choose UINT16 for it. And then the above pointer
punning is wrong, because the callee will overwrite only part of the
callee's data (Message.type is UINT32).
But, I'll assume that in edk2 / BaseTools, the enum is actually equated
with UINT32. So in practice this should work.
>
> Error:
> if (Status != XENSTORE_STATUS_SUCCESS) {
> @@ -843,7 +843,7 @@ Error:
> }
>
> /* Reply is either error or an echo of our request message type. */
> - ASSERT (Message.type == RequestType);
> + ASSERT ((enum xsd_sockmsg_type)Message.type == RequestType);
Hrmpf. Again, the same difference. Given that we decided to cast, I'd
have cast the RHS to the wider UINT32 type; considering that the RHS is
"trusted" (comes from the caller) and the LHS comes from xenstored in
dom0. (Which we shouldn't narrow before investigating.)
>
> if (ResultPtr) {
> *ResultPtr = Return;
> @@ -975,7 +975,7 @@ XenStoreWaitWatch (
> if (Message->u.Watch.Handle == Token) {
> RemoveEntryList (Entry);
> EfiReleaseLock (&xs.WatchEventsLock);
> - FreePool(Message->u.Watch.Vector);
> + FreePool((VOID*)Message->u.Watch.Vector);
> FreePool(Message);
> return XENSTORE_STATUS_SUCCESS;
> }
> @@ -1057,8 +1057,8 @@ XenStoreInit (
>
> xs.Dev = Dev;
>
> - xs.EventChannel = XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_EVTCHN);
> - XenStoreGpfn = XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_PFN);
> + xs.EventChannel = (evtchn_port_t)XenHypercallHvmGetParam (Dev,
> HVM_PARAM_STORE_EVTCHN);
> + XenStoreGpfn = (UINTN)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_PFN);
> xs.XenStore = (VOID *) (XenStoreGpfn << EFI_PAGE_SHIFT);
> DEBUG ((EFI_D_INFO, "XenBusInit: XenBus rings @%p, event channel %x\n",
> xs.XenStore, xs.EventChannel));
> @@ -1115,7 +1115,7 @@ XenStoreDeinit (
> XENSTORE_MESSAGE *Message = XENSTORE_MESSAGE_FROM_LINK (Entry);
> Entry = GetNextNode (&xs.WatchEvents, Entry);
> RemoveEntryList (&Message->Link);
> - FreePool (Message->u.Watch.Vector);
> + FreePool ((VOID*)Message->u.Watch.Vector);
> FreePool (Message);
> }
> }
> @@ -1202,7 +1202,7 @@ XenStorePathExists (
> if (Status != XENSTORE_STATUS_SUCCESS) {
> return FALSE;
> }
> - FreePool (TempStr);
> + FreePool ((VOID*)TempStr);
> return TRUE;
> }
>
> @@ -1283,7 +1283,7 @@ XenStoreTransactionStart (
> Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL,
> (VOID **) &IdStr);
> if (Status == XENSTORE_STATUS_SUCCESS) {
> - Transaction->Id = AsciiStrDecimalToUintn (IdStr);
> + Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr);
> FreePool (IdStr);
> }
>
> @@ -1419,7 +1419,7 @@ XenStoreUnregisterWatch (
> Entry = GetNextNode (&xs.WatchEvents, Entry);
> if (Message->u.Watch.Handle == Watch) {
> RemoveEntryList (&Message->Link);
> - FreePool (Message->u.Watch.Vector);
> + FreePool ((VOID*)Message->u.Watch.Vector);
> FreePool (Message);
> }
> }
>
I realize that this code is very stable (it is based on very stable
code, anyway); and I won't pretend to know it better than you. I would
have followed a very different style for this, but that's not really
important. I can't point out anything that would be a bug in practice,
so let's not waste our time.
Acked-by: Laszlo Ersek <[email protected]>
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
