avrdude
avrdude copied to clipboard
avrdudes
[bug #35265] Findings reported by cppcheck
Rene Liebscher <None> Sun 08 Jan 2012 05:18:52 PM UTC
When running cppcheck the following findings are reported:
[avr.c:94]: (information) The scope of the variable 'r' can be reduced
[avrftdi.c:368]: (information) The scope of the variable 'n' can be reduced
[avrftdi.c:404]: (style) Variable 'vid' is assigned a value that is never used
[avrftdi.c:404]: (style) Variable 'pid' is assigned a value that is never used
[avrftdi.c:404]: (style) Variable 'interface' is assigned a value that is never used
[avrftdi.c:404]: (style) Variable 'snfound' is assigned a value that is never used
[avrftdi.c:405]: (style) Unused variable: serial
[avrftdi.c:405]: (style) Variable 'foundsn' is assigned a value that is never used
[avrftdi.c:406]: (style) Unused variable: devlist
[avrftdi.c:407]: (style) Unused variable: devlist_ptr
[avrftdi.c:408]: (style) Unused variable: found_dev
[avrpart.c:335]: (information) The scope of the variable 'i' can be reduced
[avrpart.c:335]: (information) The scope of the variable 'j' can be reduced
[bitbang.c:499]: (information) The scope of the variable 'i' can be reduced
[bitbang.c:535]: (information) The scope of the variable 'tries' can be reduced
[bitbang.c:536]: (information) The scope of the variable 'i' can be reduced
[config_gram.c:1952]: (error) Memory leak: yyptr
[config_gram.c]: (information) Interrupted checking because of too many #ifdef configurations.
[jtagmkI.c:706]: (information) The scope of the variable 'b' can be reduced
[jtagmkII.c:486]: (style) Variable 'checksum' is assigned a value that is never used
[jtagmkII.c:2561]: (style) Variable 'config0' is assigned a value that is never used
[jtagmkII.c:2561]: (style) Variable 'config1' is assigned a value that is never used
[jtagmkII.c:2470]: (information) The scope of the variable 'clk' can be reduced
[jtagmkII.c:2561]: (information) The scope of the variable 'config1' can be reduced
[lexer.c:377]: (style) struct or union member 'yy_trans_info::yy_verify' is never used
[lexer.c:378]: (style) struct or union member 'yy_trans_info::yy_nxt' is never used
[lexer.c]: (information) Interrupted checking because of too many #ifdef configurations.
[par.c:49]: (style) struct or union member 'ppipins_t::pin' is never used
[ser_posix.c:197]: (style) Found obsolete function 'gethostbyname'. It is recommended that new applications use the 'getaddrinfo' function
[ser_win32.c:231]: (information) The scope of the variable 'c' can be reduced
[ser_win32.c:285]: (information) The scope of the variable 'c' can be reduced
[stk500.c:751]: (style) Variable 'flash' is assigned a value that is never used
[stk500.c:413]: (information) The scope of the variable 'rc' can be reduced
[stk500.c:992]: (information) The scope of the variable 'fosc' can be reduced
[stk500.c:996]: (information) The scope of the variable 'idx' can be reduced
[stk500v2.c:1721]: (style) Variable 'hiaddr' is assigned a value that is never used
[stk500v2.c:1854]: (style) Variable 'hiaddr' is assigned a value that is never used
[stk500v2.c:2205]: (information) The scope of the variable 'fosc' can be reduced
[stk500v2.c:2210]: (information) The scope of the variable 'idx' can be reduced
[term.c:210]: (style) Variable 'i' is assigned a value that is never used
[term.c:527]: (information) The scope of the variable 'i' can be reduced
[update.c:39]: (information) The scope of the variable 'c' can be reduced
[update.c:207]: (information) The scope of the variable 'vsize' can be reduced
[usb_libusb.c:389]: (information) The scope of the variable 'i' can be reduced
[wiring.c:154]: (information) The scope of the variable 'timetosnooze' can be reduced
Also there are a lot of findings "(style) Found obsolete function 'usleep'. It is recommended that new applications use the 'nanosleep' or 'setitimer' function", which I filtered out.
Some errors were already removed in svn revision 1035.
This issue was migrated from https://savannah.nongnu.org/bugs/?35265
Joerg Wunsch <joerg_wunsch> Sun 08 Jan 2012 10:37:44 PM UTC
Could you please post you commandline?
(style) Found obsolete function 'usleep'
Maybe we should rename our usleep() into microsleep().
Rene Liebscher
command line was 'cppcheck --enable=all .'
cppcheck version is 'Cppcheck 1.49'
'Our' usleep is the old posix usleep (except on Windows were we have a replacement in ppiwin.c).
But: POSIX.1-2001 declares this function obsolete; use nanosleep(2) instead. POSIX.1-2008 removes the specification of usleep(). (see https://www.kernel.org/doc/man-pages/online/pages/man3/usleep.3.html)
So we better replace all occurrences of usleep by an own wrapper microsleep. To use nanosleep we have check it in the configure file.
I would put this wrapper in a new C-file (avrdude.c?, as we have an avrdude.h defining a usleep for Windows.) I also would move the implementations of usleep and gettimeofday from ppiwin.c to this new file as they are used not only by the parallel port programmers.
Joerg Wunsch <joerg_wunsch> Mon 09 Jan 2012 10:27:47 PM UTC
OK, thanks, with the commandline, I can reproduce the messages now.
'Our' usleep is the old posix usleep
You're right. While I don't think any of the actual systems are going to remove it in the near future, it certainly makes sense to replace it by nanosleep().
Given that nanosleep() is quoted (in FreeBSD's manpage) to originate in Posix.1 (IEEE Std 1003.1b-1993), are there any actual systems around we want to be portable to (various Linuxes, *BSD, Solaris, MacOS X) which don't have it?
I agree about the new implementation file.
Joerg Wunsch <joerg_wunsch> Tue 10 Jan 2012 07:18:18 AM UTC
I agree about the new implementation file.
With one exception though: avrdude.c is just too generic a name. That name should probably be the name of the main (CLI) file (rather than main.c).
My suggestion is to name it util.c or such.
avrdude.h should simply combine the exported interfaces (by libavrdude.a) from all the existing header files. The current header file structure is a nuisance for anyone who wants to use the library. If there are library-internal interfaces that are not supposed to be exported (so they could be easily changed between library versions), they could go into a second header file that is only used to build the library itself, but not going to be installed even in case we once install the library itself.
Rene Liebscher
I made a first patch which replaces all usleep by microsleep and adds a file util.c which realizes microsleep either by nanosleep, usleep or by a Windows specififc function. (For the last it moved the code from ppiwin.c to util.c)
MinGW has only usleep but no nanosleep (at least configure does not find it) so I left the usleep variant in the code.
Not so sure if the project wants to adopt static code analysis tools like Coverity. It is free for Open Source projects.
libusb project uses Coverity and it does catch some errors. https://scan.coverity.com/projects/libusb-libusb
Initial fixes have been merged. https://github.com/avrdudes/avrdude/commit/d34978a124f3e4762dccc8182e93214d528c4e0c
util.c is probably no longer required.
/*
* avrdude - A Downloader/Uploader for AVR device programmers
* Copyright (C) 2003, 2004, 2006
* Eric B. Weddington <[email protected]>
* Copyright 2008, Joerg Wunsch
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
/* $Id: ppiwin.c 936 2010-01-22 16:40:17Z joerg_wunsch $ */
/*
This is the parallel port interface for Windows built using Cygwin.
In the ppi_* functions that access the parallel port registers,
fd = parallel port address
reg = register as defined in an enum in ppi.h. This must be converted
to a proper offset of the base address.
*/
#include "ac_cfg.h"
#include "avrdude.h"
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <time.h>
#if defined (WIN32NATIVE)
/* why is this double include needed ? */
#include <windows.h>
#include <sys/time.h>
#include <windows.h>
#else
#include <sys/time.h>
#endif
#if !defined(HAVE_GETTIMEOFDAY) && defined (WIN32NATIVE)
struct timezone;
int gettimeofday(struct timeval *tv, struct timezone *unused){
// i've found only ms resolution, avrdude expects us
SYSTEMTIME st;
GetSystemTime(&st);
tv->tv_sec=(long)(st.wSecond+st.wMinute*60+st.wHour*3600);
tv->tv_usec=(long)(st.wMilliseconds*1000);
return 0;
}
#endif /* HAVE_GETTIMEOFDAY */
#if defined(HAVE_NANOSLEEP)
int microsleep(unsigned int us) {
struct timespec req = {us / 1000000, (us % 1000000) * 1000};
struct timespec rem;
while( nanosleep(&req, &rem) == -1) {
if (errno != EINTR) {
return -1;
}
/* there is time left to sleep */
req = rem;
}
return 0;
}
#elif defined(HAVE_USLEEP)
int microsleep(unsigned int us) {
return usleep(us);
}
#elif defined (WIN32NATIVE)
// #define W32USLEEPDBG
#ifdef W32USLEEPDBG
# define DEBUG_QueryPerformanceCounter(arg) QueryPerformanceCounter(arg)
# define DEBUG_DisplayTimingInfo(start, stop, freq, us, has_highperf) \
do { \
unsigned long dt; \
dt = (unsigned long)((stop.QuadPart - start.QuadPart) * 1000 * 1000 \
/ freq.QuadPart); \
fprintf(stderr, \
"hpt:%i usleep usec:%lu sleep msec:%lu timed usec:%lu\n", \
has_highperf, us, ((us + 999) / 1000), dt); \
} while (0)
#else
# define DEBUG_QueryPerformanceCounter(arg)
# define DEBUG_DisplayTimingInfo(start, stop, freq, us, has_highperf)
#endif
int microsleep(unsigned int us)
{
int has_highperf;
LARGE_INTEGER freq,start,stop,loopend;
// workaround: although usleep is very precise if using
// high-performance-timers there are sometimes problems with
// verify - increasing the delay helps sometimes but not
// realiably. There must be some other problem. Maybe just
// with my test-hardware maybe in the code-base.
//// us=(unsigned long) (us*1.5);
has_highperf=QueryPerformanceFrequency(&freq);
//has_highperf=0; // debug
if (has_highperf) {
QueryPerformanceCounter(&start);
loopend.QuadPart=start.QuadPart+freq.QuadPart*us/(1000*1000);
do {
QueryPerformanceCounter(&stop);
} while (stop.QuadPart<=loopend.QuadPart);
}
else {
DEBUG_QueryPerformanceCounter(&start);
Sleep(1);
Sleep( (DWORD)((us+999)/1000) );
DEBUG_QueryPerformanceCounter(&stop);
}
DEBUG_DisplayTimingInfo(start, stop, freq, us, has_highperf);
return 0;
}
#else
#error Do not know how to implement microsleep().
#endif /* !HAVE_USLEEP */
Close this for now. I will try to set up coverity scan. https://scan.coverity.com/projects/avrdude/