Olivier, I have some questions about your patch submission. The majority are inserted, inline, into your quoted message below. Any patch marked '<DVM> Accepted.' has been committed as Reviewed by: Daryl McDaniel <daryl.mcdan...@intel.com>.
The remainder require further discussion. All of my comments are prefixed with <DVM>. <DVM> Were these all "set but not used" warnings? If not, what errors were you seeing and what version of GCC were you using? Daryl McDaniel -----Original Message----- From: Olivier Martin [mailto:olivier.mar...@arm.com] Sent: Wednesday, October 29, 2014 3:01 AM To: Mcdaniel, Daryl Cc: edk2-devel@lists.sourceforge.net; Olivier Martin Subject: [PATCH] StdLib/LibC: Removed/Fixed unused variable to build with GCC Some of the variables are unused that trigger a GCC warning/error. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Olivier Martin <olivier.mar...@arm.com> --- StdLib/BsdSocketLib/bind.c | 3 +++ StdLib/BsdSocketLib/getnameinfo.c | 8 ++++++-- StdLib/BsdSocketLib/getsockopt.c | 3 +++ StdLib/BsdSocketLib/listen.c | 3 +++ StdLib/BsdSocketLib/poll.c | 3 +++ StdLib/BsdSocketLib/res_comp.c | 4 ++-- StdLib/BsdSocketLib/res_mkupdate.c | 3 +-- StdLib/BsdSocketLib/res_update.c | 4 ---- StdLib/BsdSocketLib/setsockopt.c | 3 +++ StdLib/EfiSocketLib/Ip4.c | 12 ------------ StdLib/EfiSocketLib/Socket.c | 1 + StdLib/EfiSocketLib/Tcp4.c | 4 ---- StdLib/EfiSocketLib/Tcp6.c | 4 ---- StdLib/LibC/Containers/Queues/Fifo.c | 2 -- StdLib/LibC/Uefi/InteractiveIO/IIOutilities.c | 10 ++++++---- StdLib/LibC/Uefi/InteractiveIO/IIOwrite.c | 3 --- StdLib/LibC/Uefi/InteractiveIO/NonCanonRead.c | 3 +-- StdLib/LibC/Uefi/writev.c | 4 ++-- 18 files changed, 34 insertions(+), 43 deletions(-) diff --git a/StdLib/BsdSocketLib/bind.c b/StdLib/BsdSocketLib/bind.c index f544c94..f22fe2f 100644 --- a/StdLib/BsdSocketLib/bind.c +++ b/StdLib/BsdSocketLib/bind.c @@ -65,6 +65,9 @@ bind ( name, namelen, &errno ); + if (EFI_ERROR (Status)) { + errno = -1; + } } <DVM> errno is set in the pfnBind call. Are you seeing that errors occur yet errno doesn't get set? Also, -1 is ERESTART which indicates that a system call needs to be restarted. Are you sure you don't want that instead of one of the other predefined errors? // diff --git a/StdLib/BsdSocketLib/getnameinfo.c b/StdLib/BsdSocketLib/getnameinfo.c index fab3460..0318d93 100644 --- a/StdLib/BsdSocketLib/getnameinfo.c +++ b/StdLib/BsdSocketLib/getnameinfo.c @@ -411,15 +411,19 @@ ip6_sa2str( int flags ) { - unsigned int ifindex; +#if 0 + unsigned int ifindex; const struct in6_addr *a6; +#endif int n; _DIAGASSERT(sa6 != NULL); _DIAGASSERT(buf != NULL); - ifindex = (unsigned int)sa6->sin6_scope_id; +#if 0 + ifindex = (unsigned int)sa6->sin6_scope_id; a6 = &sa6->sin6_addr; +#endif #ifdef NI_NUMERICSCOPE if ((flags & NI_NUMERICSCOPE) != 0) { <DVM> Accepted. diff --git a/StdLib/BsdSocketLib/getsockopt.c b/StdLib/BsdSocketLib/getsockopt.c index 47b7c6f..fdbd6f7 100644 --- a/StdLib/BsdSocketLib/getsockopt.c +++ b/StdLib/BsdSocketLib/getsockopt.c @@ -60,6 +60,9 @@ getsockopt ( option_value, option_len, &errno ); + if (EFI_ERROR (Status)) { + errno = -1; + } } <DVM> Same comment as for bind(). pfnOptionGet() sets errno for all error conditions. It even sets errno to ESUCCESS, 0, if there are no errors. Please let me know if you are seeing getsockopt() failing but errno is not set. // diff --git a/StdLib/BsdSocketLib/listen.c b/StdLib/BsdSocketLib/listen.c index 7c6d5f3..d3060a4 100644 --- a/StdLib/BsdSocketLib/listen.c +++ b/StdLib/BsdSocketLib/listen.c @@ -58,6 +58,9 @@ listen ( Status = pSocketProtocol->pfnListen ( pSocketProtocol, backlog, &errno ); + if (EFI_ERROR (Status)) { + errno = -1; + } } <DVM> Same comments as for bind() and getsockopt(). pfnListen() sets errno. // diff --git a/StdLib/BsdSocketLib/poll.c b/StdLib/BsdSocketLib/poll.c index dc17567..dcc367a 100644 --- a/StdLib/BsdSocketLib/poll.c +++ b/StdLib/BsdSocketLib/poll.c @@ -49,6 +49,9 @@ BslSocketPoll ( Events, &DetectedEvents, &errno ); + if (EFI_ERROR (Status)) { + errno = -1; + } } <DVM> Like bind(), getsockopt(), and listen(), the internal pfnPoll() function sets errno if an error occurs. The only errno value this implementation supports is EINVAL which indicates that a non-socket or invalid fd was passed as the first argument. I noticed a bug in pfnPoll() where errno is not actually set to EINVAL. That will be fixed. // diff --git a/StdLib/BsdSocketLib/res_comp.c b/StdLib/BsdSocketLib/res_comp.c index cddda3e..7b5f2aa 100644 --- a/StdLib/BsdSocketLib/res_comp.c +++ b/StdLib/BsdSocketLib/res_comp.c @@ -168,7 +168,7 @@ res_hnok( const char *dn ) { - int ppch = '\0', pch = PERIOD, ch = *dn++; + int pch = PERIOD, ch = *dn++; while (ch != '\0') { int nch = *dn++; @@ -185,7 +185,7 @@ res_hnok( if (!middlechar(ch)) return (0); } - ppch = pch, pch = ch, ch = nch; + pch = ch, ch = nch; } return (1); } <DVM> Accepted. diff --git a/StdLib/BsdSocketLib/res_mkupdate.c b/StdLib/BsdSocketLib/res_mkupdate.c index 3201f31..1056fc6 100644 --- a/StdLib/BsdSocketLib/res_mkupdate.c +++ b/StdLib/BsdSocketLib/res_mkupdate.c @@ -100,7 +100,7 @@ int res_mkupdate(ns_updrec *rrecp_in, u_char *buf, int buflen) { ns_updrec *rrecp_start = rrecp_in; HEADER *hp; - u_char *cp, *sp1, *sp2, *startp, *endp; + u_char *cp, *sp2, *startp, *endp; int n, i, soanum, multiline; ns_updrec *rrecp; struct in_addr ina; @@ -125,7 +125,6 @@ res_mkupdate(ns_updrec *rrecp_in, u_char *buf, int buflen) { hp->id = htons(++_res.id); hp->opcode = ns_o_update; hp->rcode = NOERROR; - sp1 = buf + 2*INT16SZ; /* save pointer to zocount */ cp = buf + HFIXEDSZ; buflen -= HFIXEDSZ; dpp = dnptrs; <DVM> Accepted diff --git a/StdLib/BsdSocketLib/res_update.c b/StdLib/BsdSocketLib/res_update.c index a01d832..f3ff0ce 100644 --- a/StdLib/BsdSocketLib/res_update.c +++ b/StdLib/BsdSocketLib/res_update.c @@ -120,7 +120,6 @@ res_update(ns_updrec *rrecp_in) { int i, j, k = 0, n, ancount, nscount, arcount, rcode, rdatasize, newgroup, done, myzone, seen_before, numzones = 0; u_int16_t dlen, class, qclass, type, qtype; - u_int32_t ttl; if ((_res.options & RES_INIT) == 0 && res_init() == -1) { h_errno = NETDB_INTERNAL; @@ -302,7 +301,6 @@ res_update(ns_updrec *rrecp_in) { if (cp + INT32SZ + INT16SZ > eom) return (-1); /* continue processing the soa record */ - GETLONG(ttl, cp); GETSHORT(dlen, cp); if (cp + dlen > eom) return (-1); @@ -424,7 +422,6 @@ ans=%d, auth=%d, add=%d, rcode=%d\n", return (-1); GETSHORT(type, cp); GETSHORT(class, cp); - GETLONG(ttl, cp); GETSHORT(dlen, cp); if (cp + dlen > eom) return (-1); @@ -450,7 +447,6 @@ ans=%d, auth=%d, add=%d, rcode=%d\n", return (-1); GETSHORT(type, cp); GETSHORT(class, cp); - GETLONG(ttl, cp); GETSHORT(dlen, cp); if (cp + dlen > eom) return (-1); <DVM> Rejected. While ttl itself isn't used, the GETLONG(ttl, cp); invocations are required in order to advance the cp pointer over the ttl field of the header. diff --git a/StdLib/BsdSocketLib/setsockopt.c b/StdLib/BsdSocketLib/setsockopt.c index 64f3a35..7db7dea 100644 --- a/StdLib/BsdSocketLib/setsockopt.c +++ b/StdLib/BsdSocketLib/setsockopt.c @@ -59,6 +59,9 @@ setsockopt ( option_value, option_len, &errno ); + if (EFI_ERROR (Status)) { + errno = -1; + } } <DVM> Like bind(), etc. pfnOptionSet() sets errno if any error occurs. // diff --git a/StdLib/EfiSocketLib/Ip4.c b/StdLib/EfiSocketLib/Ip4.c index ed71194..46425db 100644 --- a/StdLib/EfiSocketLib/Ip4.c +++ b/StdLib/EfiSocketLib/Ip4.c @@ -242,8 +242,6 @@ EslIp4OptionSet ( ) { BOOLEAN bTrueFalse; - socklen_t LengthInBytes; - UINT8 * pOptionData; EFI_STATUS Status; DBG_ENTER ( ); @@ -257,8 +255,6 @@ EslIp4OptionSet ( // // Determine if the option protocol matches // - LengthInBytes = 0; - pOptionData = NULL; switch ( OptionName ) { default: // @@ -283,12 +279,6 @@ EslIp4OptionSet ( bTrueFalse = FALSE; } pOptionValue = &bTrueFalse; - - // - // Set the option value - // - pOptionData = (UINT8 *)&pSocket->bIncludeHeader; - LengthInBytes = sizeof ( pSocket->bIncludeHeader ); } break; } <DVM> The patch is valid as far as removing two meaningless variables. There is something else wrong with this function, though, since it does nothing other than validate that OptionName is set to IP_HDRINCL. Query submitted to sub-package owner. @@ -653,7 +643,6 @@ EslIp4RxComplete ( ) { size_t LengthInBytes; - ESL_PORT * pPort; ESL_PACKET * pPacket; EFI_IP4_RECEIVE_DATA * pRxData; EFI_STATUS Status; @@ -663,7 +652,6 @@ EslIp4RxComplete ( // // Get the operation status. // - pPort = pIo->pPort; Status = pIo->Token.Ip4Rx.Status; // <DVM> Accepted change to EslIp4RxComplete(). diff --git a/StdLib/EfiSocketLib/Socket.c b/StdLib/EfiSocketLib/Socket.c index d53473e..740602d 100644 --- a/StdLib/EfiSocketLib/Socket.c +++ b/StdLib/EfiSocketLib/Socket.c @@ -4130,6 +4130,7 @@ EslSocketPortCloseComplete ( // Status = EslSocketPortCloseRxDone ( pPort ); DBG_EXIT_STATUS ( Status ); + ASSERT_EFI_ERROR (Status); } <DVM> Query submitted to the sub-package owner. The ASSERT may be superfluous. diff --git a/StdLib/EfiSocketLib/Tcp4.c b/StdLib/EfiSocketLib/Tcp4.c index 7ece38d..0419ee2 100644 --- a/StdLib/EfiSocketLib/Tcp4.c +++ b/StdLib/EfiSocketLib/Tcp4.c @@ -840,7 +840,6 @@ EslTcp4ListenComplete ( EFI_HANDLE ChildHandle; struct sockaddr_in LocalAddress; EFI_TCP4_CONFIG_DATA * pConfigData; - ESL_LAYER * pLayer; ESL_PORT * pNewPort; ESL_SOCKET * pNewSocket; ESL_SOCKET * pSocket; @@ -869,7 +868,6 @@ EslTcp4ListenComplete ( // Allocate a socket for this connection // ChildHandle = NULL; - pLayer = &mEslLayer; Status = EslSocketAllocate ( &ChildHandle, DEBUG_CONNECTION, &pNewSocket ); <DVM> Accepted. @@ -1924,7 +1922,6 @@ EslTcp4TxBuffer ( ESL_PACKET ** ppQueueHead; ESL_PACKET ** ppQueueTail; ESL_PACKET * pPreviousPacket; - ESL_TCP4_CONTEXT * pTcp4; size_t * pTxBytes; EFI_TCP4_TRANSMIT_DATA * pTxData; EFI_STATUS Status; @@ -1951,7 +1948,6 @@ EslTcp4TxBuffer ( // // Determine the queue head // - pTcp4 = &pPort->Context.Tcp4; bUrgent = (BOOLEAN)( 0 != ( Flags & MSG_OOB )); bUrgentQueue = bUrgent && ( !pSocket->bOobInLine ) <DVM> Accepted. diff --git a/StdLib/EfiSocketLib/Tcp6.c b/StdLib/EfiSocketLib/Tcp6.c index 21c4109..77a4a4c 100644 --- a/StdLib/EfiSocketLib/Tcp6.c +++ b/StdLib/EfiSocketLib/Tcp6.c @@ -871,7 +871,6 @@ EslTcp6ListenComplete ( EFI_HANDLE ChildHandle; struct sockaddr_in6 LocalAddress; EFI_TCP6_CONFIG_DATA * pConfigData; - ESL_LAYER * pLayer; ESL_PORT * pNewPort; ESL_SOCKET * pNewSocket; ESL_SOCKET * pSocket; @@ -900,7 +899,6 @@ EslTcp6ListenComplete ( // Allocate a socket for this connection // ChildHandle = NULL; - pLayer = &mEslLayer; Status = EslSocketAllocate ( &ChildHandle, DEBUG_CONNECTION, &pNewSocket ); <DVM> Accepted. @@ -1993,7 +1991,6 @@ EslTcp6TxBuffer ( ESL_PACKET ** ppQueueHead; ESL_PACKET ** ppQueueTail; ESL_PACKET * pPreviousPacket; - ESL_TCP6_CONTEXT * pTcp6; size_t * pTxBytes; EFI_TCP6_TRANSMIT_DATA * pTxData; EFI_STATUS Status; @@ -2020,7 +2017,6 @@ EslTcp6TxBuffer ( // // Determine the queue head // - pTcp6 = &pPort->Context.Tcp6; bUrgent = (BOOLEAN)( 0 != ( Flags & MSG_OOB )); bUrgentQueue = bUrgent && ( !pSocket->bOobInLine ) <DVM> Accepted. diff --git a/StdLib/LibC/Containers/Queues/Fifo.c b/StdLib/LibC/Containers/Queues/Fifo.c index 73254d2..93b62f8 100644 --- a/StdLib/LibC/Containers/Queues/Fifo.c +++ b/StdLib/LibC/Containers/Queues/Fifo.c @@ -278,7 +278,6 @@ FIFO_Dequeue ( { UINTN QPtr; UINT32 RDex; - UINT32 SizeOfElement; UINT32 i; assert(Self != NULL); @@ -289,7 +288,6 @@ FIFO_Dequeue ( } else { RDex = Self->ReadIndex; // Get this FIFO's Read Index - SizeOfElement = Self->ElementSize; // Get size of this FIFO's elements Count = MIN(Count, Self->Count(Self, AsElements)); // Lesser of requested or actual QPtr = (UINTN)Self->Queue + (RDex * Self->ElementSize); // Point to Read location in FIFO <DVM> Replacing all of the "Self->ElementSize" clauses in the function with "SizeOfElement" would be more efficient. diff --git a/StdLib/LibC/Uefi/InteractiveIO/IIOutilities.c b/StdLib/LibC/Uefi/InteractiveIO/IIOutilities.c index 1c978ea..b498d2b 100644 --- a/StdLib/LibC/Uefi/InteractiveIO/IIOutilities.c +++ b/StdLib/LibC/Uefi/InteractiveIO/IIOutilities.c @@ -75,7 +75,7 @@ IIO_GetInChar ( { cIIO *This; cFIFO *InBuf; - EFI_STATUS Status; + size_t Status; ssize_t NumRead; wint_t RetVal; wchar_t InChar; @@ -92,8 +92,10 @@ IIO_GetInChar ( } if(BufCnt > 0) { Status = InBuf->Read(InBuf, &InChar, 1); - --BufCnt; - NumRead = 1; + if (Status > 0) { + --BufCnt; + NumRead = 1; + } } else { NumRead = filp->f_ops->fo_read(filp, &filp->f_offset, sizeof(wchar_t), &InChar); @@ -104,7 +106,7 @@ IIO_GetInChar ( else { RetVal = (wint_t)InChar; } - return InChar; + return RetVal; } /** Get the current cursor position. <DVM> Accepted. diff --git a/StdLib/LibC/Uefi/InteractiveIO/IIOwrite.c b/StdLib/LibC/Uefi/InteractiveIO/IIOwrite.c index 927f4f4..2c5e81d 100644 --- a/StdLib/LibC/Uefi/InteractiveIO/IIOwrite.c +++ b/StdLib/LibC/Uefi/InteractiveIO/IIOwrite.c @@ -63,7 +63,6 @@ IIO_WriteOne(struct __filedes *filp, cFIFO *OBuf, wchar_t InCh) UINT32 CurRow; // Current cursor row on the screen UINT32 PrevColumn; // Previous column. Used to detect wrapping. UINT32 AdjColumn; // Current cursor column on the screen - UINT32 AdjRow; // Current cursor row on the screen RetVal = -1; wcb = wc; @@ -79,7 +78,6 @@ IIO_WriteOne(struct __filedes *filp, cFIFO *OBuf, wchar_t InCh) CurRow = This->CurrentXY.Row; numW = 1; // The majority of characters buffer one character - AdjRow = 0; // Most characters just cause horizontal movement AdjColumn = 0; if(OFlag & OPOST) { /* Perform output processing */ @@ -127,7 +125,6 @@ IIO_WriteOne(struct __filedes *filp, cFIFO *OBuf, wchar_t InCh) numW = 2; CurColumn = 0; } - AdjRow = 1; break; //}} case CHAR_BACKSPACE: //{{ <DVM> Accepted. diff --git a/StdLib/LibC/Uefi/InteractiveIO/NonCanonRead.c b/StdLib/LibC/Uefi/InteractiveIO/NonCanonRead.c index bcae9c8..3b226d1 100644 --- a/StdLib/LibC/Uefi/InteractiveIO/NonCanonRead.c +++ b/StdLib/LibC/Uefi/InteractiveIO/NonCanonRead.c @@ -37,7 +37,6 @@ IIO_NonCanonRead ( cIIO *This; cFIFO *InBuf; struct termios *Termio; - EFI_STATUS Status; ssize_t NumRead; cc_t tioMin; cc_t tioTime; @@ -74,7 +73,7 @@ IIO_NonCanonRead ( if(InBuf->IsEmpty(InBuf)) { NumRead = filp->f_ops->fo_read(filp, &filp->f_offset, sizeof(wchar_t), &InChar); if(NumRead > 0) { - Status = InBuf->Write(InBuf, &InChar, 1); // Buffer the character + InBuf->Write(InBuf, &InChar, 1); // Buffer the character } } // break; <DVM> Accepted. diff --git a/StdLib/LibC/Uefi/writev.c b/StdLib/LibC/Uefi/writev.c index 9cff086..56712c5 100644 --- a/StdLib/LibC/Uefi/writev.c +++ b/StdLib/LibC/Uefi/writev.c @@ -101,7 +101,7 @@ writev( ) { const struct iovec *pVecTmp; - char *pBuf, *pBufTmp; + char *pBuf; size_t TotalBytes, i, ret; // @@ -126,7 +126,7 @@ writev( // Copy vectors to the buffer // - for (pBufTmp = pBuf; iovcnt; iovcnt--) { + for (; iovcnt; iovcnt--) { bcopy(iov->iov_base, pBuf, iov->iov_len); pBuf += iov->iov_len; iov++; <DVM> Accepted. -- 2.1.1 ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
