Opened 8 years ago
Closed 8 years ago
#5293 closed Bug (fixed)
Wrong error codes used in upnp.c
Reported by: | rb07 | Owned by: | jordan |
---|---|---|---|
Priority: | Normal | Milestone: | 2.77 |
Component: | libtransmission | Version: | 2.76 |
Severity: | Normal | Keywords: | |
Cc: |
Description
It seems that the wrong variable, and wrong error codes are currently being used in tr_upnpDiscover().
It should be something like this:
Index: libtransmission/upnp.c =================================================================== --- libtransmission/upnp.c (revision 14017) +++ libtransmission/upnp.c (working copy) @@ -98,11 +98,11 @@ #elif defined (HAVE_MINIUPNP_15) ret = upnpDiscover (msec, NULL, NULL, 0); #else - ret = UPNPCOMMAND_UNKNOWN_ERROR; + ret = UPNPDISCOVER_UNKNOWN_ERROR; #endif - if (ret != UPNPCOMMAND_SUCCESS) - tr_logAddNamedDbg (getKey (), "upnpDiscover failed (errno %d - %s)", err, tr_strerror (err)); + if (err != UPNPDISCOVER_SUCCESS) + tr_logAddNamedDbg (getKey (), "upnpDiscover failed (errno %d - %s)", errno, tr_strerror (errno)); return ret; }
this is assuming errno is preserved by the underlying function, since its not touched, and the value returned in err doesn't have the OS error code.
BTW version 1.8 of miniupnpc was released this week, no changes w.r.t. this problem or the API (except perhaps to change the guards to include the newer versions).
Attachments (1)
Change History (9)
comment:1 Changed 8 years ago by jordan
- Status changed from new to assigned
comment:2 Changed 8 years ago by jordan
- Milestone changed from None Set to 2.80
- Resolution set to fixed
- Status changed from assigned to closed
comment:3 Changed 8 years ago by jordan
- Milestone changed from 2.80 to 2.77
comment:4 Changed 8 years ago by taem
- Resolution fixed deleted
- Status changed from closed to reopened
comment:5 Changed 8 years ago by taem
Hi,
Can't build the code with r14020
gcc -DPACKAGE_NAME=\"transmission\" -DPACKAGE_TARNAME=\"transmission\" -DPACKAGE_VERSION=\"2.76+\" -DPACKAGE_STRING=\"transmission\ 2.76+\" -DPACKAGE_BUGREPORT=\"http://trac.transmissionbt.com/newticket\" -DPACKAGE_URL=\"\" -DPACKAGE=\"transmission\" -DVERSION=\"2.76+\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DSTDC_HEADERS=1 -DTIME_WITH_SYS_TIME=1 -DHAVE_STDBOOL_H=1 -DHAVE_ICONV_OPEN=1 -DHAVE_PREAD=1 -DHAVE_PWRITE=1 -DHAVE_DAEMON=1 -DHAVE_DIRNAME=1 -DHAVE_BASENAME=1 -DHAVE_STRCASECMP=1 -DHAVE_LOCALTIME_R=1 -DHAVE_FALLOCATE64=1 -DHAVE_POSIX_FALLOCATE=1 -DHAVE_MEMMEM=1 -DHAVE_STRSEP=1 -DHAVE_STRTOLD=1 -DHAVE_SYSLOG=1 -DHAVE_VALLOC=1 -DHAVE_GETPAGESIZE=1 -DHAVE_POSIX_MEMALIGN=1 -DHAVE_STATVFS=1 -DHAVE_MKDTEMP=1 -DHAVE_PTHREAD=1 -DHAVE__TMP_DUMMY1_ZLIB_H=1 -DHAVE_ZLIB=1 -D_FILE_OFFSET_BITS=64 -DHAVE_LSEEK64=1 -DHAVE_GETMNTENT=1 -DHAVE_DECL_POSIX_FADVISE=1 -DHAVE_POSIX_FADVISE=1 -DWITH_INOTIFY=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_XFS_XFS_H=1 -DWITH_UTP=1 -DHAVE_MINIUPNP_15=1 -DSYSTEM_MINIUPNP=1 -DHAVE_LIBINTL_H=1 -DGETTEXT_PACKAGE=\"transmission-gtk\" -DHAVE_LOCALE_H=1 -DHAVE_LC_MESSAGES=1 -DHAVE_BIND_TEXTDOMAIN_CODESET=1 -DHAVE_GETTEXT=1 -DHAVE_DCGETTEXT=1 -DENABLE_NLS=1 -I. -I.. -D__TRANSMISSION__ -DPACKAGE_DATA_DIR=\""/usr/share"\" -D_FORTIFY_SOURCE=2 -I../third-party/dht -I../third-party/ -pthread -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -g -O0 -std=gnu99 -ggdb3 -Wall -W -Wpointer-arith -Wformat-security -Wcast-align -Wundef -Wcast-align -Wstrict-prototypes -Wmissing-declarations -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -Wunused-parameter -Wwrite-strings -Winline -Wfloat-equal -Wextra -Wdeclaration-after-statement -Winit-self -Wvariadic-macros -c upnp.c upnp.c: In function ‘tr_upnpDiscover’: upnp.c:94:15: error: ‘UPNPDISCOVER_SUCCESS’ undeclared (first use in this function) upnp.c:94:15: note: each undeclared identifier is reported only once for each function it appears in make[3]: *** [upnp.o] Error 1 make[3]: Leaving directory `/home/taem/src/transmission/transmission/libtransmission' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/home/taem/src/transmission/transmission' dh_auto_build: make -j1 returned exit code 2 make[1]: *** [override_dh_auto_build] Error 2 make[1]: Leaving directory `/home/taem/src/transmission/transmission' make: *** [build] Error 2
$ dpkg -l | grep miniupnp ii libminiupnpc-dev 1.5-2 i386 UPnP IGD client lightweight library development files ii libminiupnpc5 1.5-2 i386 UPnP IGD client lightweight library
comment:6 Changed 8 years ago by rb07
Oops! upnp.c patch take two (added).
But looking at miniupnc's code, in particular their command line tool: upnpc.c, it doesn't use errno at all, just test if ret is NULL, and print their error code (which is one of 3: unknown-, socket-, memory- error). I don't know if logging errno will be usefull.
For debugging purposes the return from UPNP_GetValidIGD() is more interesting, its used to show:
case 1: printf("Found valid IGD : %s\n", urls.controlURL); break; case 2: printf("Found a (not connected?) IGD : %s\n", urls.controlURL); printf("Trying to continue anyway\n"); break; case 3: printf("UPnP device found. Is it an IGD ? : %s\n", urls.controlURL); printf("Trying to continue anyway\n"); break; default: printf("Found device (igd ?) : %s\n", urls.controlURL); printf("Trying to continue anyway\n");
Changed 8 years ago by rb07
comment:7 Changed 8 years ago by taem
rb07, the patch fixed FTBFS. Thanks.
comment:8 Changed 8 years ago by jordan
- Resolution set to fixed
- Status changed from reopened to closed
if we're going to do the symbolic values of "err" correctly, we shouldn't initialize it to zero either. :)
r14020: "(libT) #5293 'Wrong error codes used in upnp.c': patch by rb07"