Date: Sat, 7 May 2011 19:09:03 -0700 From: Garrett Cooper <yanegomi@gmail.com> To: freebsd-hackers@freebsd.org Subject: Re: [PATCH] draft patch to make usr.bin/kdump WARNS?= 6 clean Message-ID: <BANLkTimGjDovv=SAYw98m4L=xOhojN1anQ@mail.gmail.com> In-Reply-To: <BANLkTin4ip5gp3gQJVz3dvONtW8cH7puCg@mail.gmail.com> References: <BANLkTinR1qXERz9QJfneM4aKXhdLdz3ZtQ@mail.gmail.com> <BANLkTiniKkdQYs7vyWmoeYDjJMH%2B0oAFRg@mail.gmail.com> <BANLkTimAZ3E6BNT606z_AN0uYGuVrN9N3A@mail.gmail.com> <BANLkTikHBVWkRDPBjSCkXLm-%2BUd-KisZgw@mail.gmail.com> <BANLkTin4ip5gp3gQJVz3dvONtW8cH7puCg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Mon, May 2, 2011 at 9:40 AM, Garrett Cooper <yanegomi@gmail.com> wrote:
> On Mon, May 2, 2011 at 9:24 AM, Garrett Cooper <yanegomi@gmail.com> wrote:
>> ---------- Forwarded message ----------
>> From: Garrett Cooper <yanegomi@gmail.com>
>> Date: Mon, May 2, 2011 at 9:24 AM
>> Subject: Re: [PATCH] draft patch to make usr.bin/kdump WARNS?= 6 clean
>> To: Arnaud Lacombe <lacombar@gmail.com>
>>
>>
>> On Mon, May 2, 2011 at 9:21 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>>> Hi,
>>>
>>> On Mon, May 2, 2011 at 12:10 PM, Garrett Cooper <yanegomi@gmail.com> wrote:
>>>> I wanted to do something different this weekend, and I picked
>>>> usr.bin/kdump as a likely 'victim' for converting from WARNS?= 0 to
>>>> WARNS?= 6. I'm curious as to whether or not this is on the right
>>>> track, but here's the reasoning I used:
>>>>
>>>> 1. Conditionally include diskmbr.h or diskpc98.h based on whether or
>>>> not an architecture was non-pc98 or pc98 to avoid duplicate
>>>> definitions, because the beforementioned headers are mutually
>>>> exclusive.
>>>> 2. Move the sockfamilyname declaration to kdump_subr.h as it's used in
>>>> the generated ioctl.c file.
>>>> 3. Fix a signed vs unsigned comparison with a simple cast because the
>>>> size_t value will be sufficiently small that it can be converted to a
>>>> signed comparison.
>>>> 4. Fix a cast assignment type source//dest value alignment issue on
>>>> ia64 assigning a struct sockaddr value to either struct sockaddr_in or
>>>> struct sockaddr_in6 by using calloc and memcpy.
>>>> 5. Fix structure alignment issues on arm by marking some structures as __packed.
>>>> 6. Fix a shadowed declaration for flags by renaming a locally scoped
>>>> variable to _flags; add appropriate type to field.
>>>> 7. Remove unused argument to ktruser_malloc.
>>>> 8. Add missing declarations for ktruser_malloc and ktruser_rtld.
>>>>
>>>> I've run some basic tests and things seem sane (in particular
>>>> ktrace'ing ktrace :)... ktrace'ing 'ssh ::1' and ktrace'ing 'ssh
>>>> localhost', but I was wondering if there was anything I was missing or
>>>> if someone else who ran arm or ia64 could test this patch out for me.
>>>> I've run make universe on amd64, i386, ia64, mips, and pc98, and
>>>> things seem sane, but I can't play around with those machines to
>>>> determine whether or not they're functional at runtime with the above
>>>> changes.
>>>> Thanks!
>>>> -Garrett
>>>>
>>> I do not see any patch, either inline or attached.
>>>
>>> - Arnaud
>>>
>>>> PS Oh yeah... no commit bit means that I can't commit this either, but
>>>> I was curious if my approach was correct before getting to that step
>>>> :).
>>
>> Yeah... I'm stupid for not attaching it. Need to get more sleep.
>
> Note to self: should be freeing socket structures after use, and I
> should apply similar logic to the rest of the socket inspection code.
> I'll attach another version after I do some more testing.
Here's an updated patch that was run through make universe a
couple of times.
Thanks!
-Garrett
[-- Attachment #2 --]
Index: usr.bin/kdump/kdump.c
===================================================================
--- usr.bin/kdump/kdump.c (revision 221219)
+++ usr.bin/kdump/kdump.c (working copy)
@@ -99,8 +99,9 @@
void ktrsockaddr(struct sockaddr *);
void ktrstat(struct stat *);
void ktrstruct(char *, size_t);
+void ktruser_malloc(unsigned char * p);
+void ktruser_rtld(int len, unsigned char * p);
void usage(void);
-void sockfamilyname(int);
const char *ioctlname(u_long);
int timestamp, decimal, fancy = 1, suppressdata, tail, threads, maxdata,
@@ -528,13 +529,13 @@
ip++;
narg--;
} else if (ktr->ktr_code == SYS_open) {
- int flags;
+ int open_flags;
int mode;
print_number(ip,narg,c);
- flags = *ip;
+ open_flags = *ip;
mode = *++ip;
(void)putchar(',');
- flagsandmodename (flags, mode, decimal);
+ flagsandmodename (open_flags, mode, decimal);
ip++;
narg-=2;
} else if (ktr->ktr_code == SYS_wait4) {
@@ -1167,7 +1168,7 @@
size_t mapsize;
int refcnt;
char name[MAXPATHLEN];
-};
+} __packed;
void
ktruser_rtld(int len, unsigned char *p)
@@ -1253,10 +1254,10 @@
void *p;
size_t s;
void *r;
-};
+} __packed;
void
-ktruser_malloc(int len, unsigned char *p)
+ktruser_malloc(unsigned char *p)
{
struct utrace_malloc *ut = (struct utrace_malloc *)p;
@@ -1280,7 +1281,7 @@
}
if (len == sizeof(struct utrace_malloc)) {
- ktruser_malloc(len, p);
+ ktruser_malloc(p);
return;
}
@@ -1315,61 +1316,67 @@
printf(", ");
#define check_sockaddr_len(n) \
- if (sa_##n->s##n##_len < sizeof(struct sockaddr_##n)) { \
+ if (sa_##n.s##n##_len < sizeof(struct sockaddr_##n)) { \
printf("invalid"); \
break; \
}
switch(sa->sa_family) {
case AF_INET: {
- struct sockaddr_in *sa_in;
+ struct sockaddr_in sa_in;
- sa_in = (struct sockaddr_in *)sa;
+ memset(&sa_in, 0, sizeof(sa_in));
+ memcpy(&sa_in, sa, sizeof(sa));
check_sockaddr_len(in);
- inet_ntop(AF_INET, &sa_in->sin_addr, addr, sizeof addr);
- printf("%s:%u", addr, ntohs(sa_in->sin_port));
+ inet_ntop(AF_INET, &sa_in.sin_addr, addr, sizeof addr);
+ printf("%s:%u", addr, ntohs(sa_in.sin_port));
break;
}
#ifdef NETATALK
case AF_APPLETALK: {
- struct sockaddr_at *sa_at;
+ struct sockaddr_at sa_at;
struct netrange *nr;
- sa_at = (struct sockaddr_at *)sa;
+ memset(&sa_at, 0, sizeof(sa_at));
+ memcpy(&sa_at, sa, sizeof(sa));
check_sockaddr_len(at);
- nr = &sa_at->sat_range.r_netrange;
- printf("%d.%d, %d-%d, %d", ntohs(sa_at->sat_addr.s_net),
- sa_at->sat_addr.s_node, ntohs(nr->nr_firstnet),
+ nr = &sa_at.sat_range.r_netrange;
+ printf("%d.%d, %d-%d, %d", ntohs(sa_at.sat_addr.s_net),
+ sa_at.sat_addr.s_node, ntohs(nr->nr_firstnet),
ntohs(nr->nr_lastnet), nr->nr_phase);
break;
}
#endif
case AF_INET6: {
- struct sockaddr_in6 *sa_in6;
+ struct sockaddr_in6 sa_in6;
- sa_in6 = (struct sockaddr_in6 *)sa;
+ memset(&sa_in6, 0, sizeof(sa_in6));
+ memcpy(&sa_in6, sa, sizeof(sa));
check_sockaddr_len(in6);
- inet_ntop(AF_INET6, &sa_in6->sin6_addr, addr, sizeof addr);
- printf("[%s]:%u", addr, htons(sa_in6->sin6_port));
+ inet_ntop(AF_INET6, &sa_in6.sin6_addr, addr, sizeof addr);
+ printf("[%s]:%u", addr, htons(sa_in6.sin6_port));
break;
}
#ifdef IPX
case AF_IPX: {
- struct sockaddr_ipx *sa_ipx;
+ struct sockaddr_ipx sa_ipx;
- sa_ipx = (struct sockaddr_ipx *)sa;
+ memset(&sa_ipx, 0, sizeof(sa_ipx));
+ memcpy(&sa_ipx, sa, sizeof(sa));
check_sockaddr_len(ipx);
/* XXX wish we had ipx_ntop */
- printf("%s", ipx_ntoa(sa_ipx->sipx_addr));
+ printf("%s", ipx_ntoa(sa_ipx.sipx_addr));
+ free(sa_ipx);
break;
}
#endif
case AF_UNIX: {
- struct sockaddr_un *sa_un;
+ struct sockaddr_un sa_un;
- sa_un = (struct sockaddr_un *)sa;
+ memset(&sa_un, 0, sizeof(sa_un));
+ memcpy(&sa_un, sa, sizeof(sa));
check_sockaddr_len(un);
- printf("%.*s", (int)sizeof(sa_un->sun_path), sa_un->sun_path);
+ printf("%.*s", (int)sizeof(sa_un.sun_path), sa_un.sun_path);
break;
}
default:
@@ -1480,8 +1487,8 @@
if (datalen == 0)
goto invalid;
/* sanity check */
- for (i = 0; i < namelen; ++i)
- if (!isalpha((unsigned char)name[i]))
+ for (i = 0; i < (int)namelen; ++i)
+ if (!isalpha(name[i]))
goto invalid;
if (strcmp(name, "stat") == 0) {
if (datalen != sizeof(struct stat))
Index: usr.bin/kdump/kdump_subr.h
===================================================================
--- usr.bin/kdump/kdump_subr.h (revision 221219)
+++ usr.bin/kdump/kdump_subr.h (working copy)
@@ -45,3 +45,4 @@
void minheritname (int);
void quotactlname (int);
void ptraceopname (int);
+void sockfamilyname(int);
Index: usr.bin/kdump/mkioctls
===================================================================
--- usr.bin/kdump/mkioctls (revision 221219)
+++ usr.bin/kdump/mkioctls (working copy)
@@ -22,12 +22,21 @@
# XXX should we use an ANSI cpp?
ioctl_includes=`
cd $1
- find -H -s * -name '*.h' |
+ find -H -s * -name '*.h' | grep -v '.*disk.*\.h' | \
xargs egrep -l \
'^#[ ]*define[ ]+[A-Za-z_][A-Za-z0-9_]*[ ]+_IO[^a-z0-9_]' |
awk '{printf("#include <%s>\\\\n", $1)}'
`
+case "`uname -m`" in
+*pc98*)
+ ioctl_includes="$ioctl_includes#include <sys/diskpc98.h>\\n"
+ ;;
+*)
+ ioctl_includes="$ioctl_includes#include <sys/diskmbr.h>\\n"
+ ;;
+esac
+
awk -v x="$ioctl_includes" 'BEGIN {print x}' |
gcc -E -I$1 -dM -DCOMPAT_43TTY - |
awk -v ioctl_includes="$ioctl_includes" -v use_switch="$use_switch" '
Index: usr.bin/kdump/Makefile
===================================================================
--- usr.bin/kdump/Makefile (revision 221219)
+++ usr.bin/kdump/Makefile (working copy)
@@ -15,7 +15,7 @@
SRCS+= linux_syscalls.c
.endif
-WARNS?= 0
+WARNS?= 6
CLEANFILES= ioctl.c kdump_subr.c linux_syscalls.c
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTimGjDovv=SAYw98m4L=xOhojN1anQ>