| Message ID | 20180313044944.21058-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Alexey Kardashevskiy, on mar. 13 mars 2018 15:49:44 +1100, wrote: > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Applied to my tree, thanks! > --- > > checkpatch.pl complains on every single changed line as it keeps > using tabs - do I need to post 's/\t/ /g'? > > --- > Changes: > v2: > * replaced static cast to (in_addr*) with temporary structs > --- > slirp/arp_table.c | 4 ++-- > slirp/socket.c | 8 ++++---- > slirp/udp.c | 4 ++-- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/slirp/arp_table.c b/slirp/arp_table.c > index 3547043..f81963b 100644 > --- a/slirp/arp_table.c > +++ b/slirp/arp_table.c > @@ -33,7 +33,7 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t ethaddr[ETH_ALEN]) > int i; > > DEBUG_CALL("arp_table_add"); > - DEBUG_ARG("ip = 0x%x", ip_addr); > + DEBUG_ARG("ip = %s", inet_ntoa((struct in_addr){.s_addr = ip_addr})); > DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n", > ethaddr[0], ethaddr[1], ethaddr[2], > ethaddr[3], ethaddr[4], ethaddr[5])); > @@ -67,7 +67,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr, > int i; > > DEBUG_CALL("arp_table_search"); > - DEBUG_ARG("ip = 0x%x", ip_addr); > + DEBUG_ARG("ip = %s", inet_ntoa((struct in_addr){.s_addr = ip_addr})); > > /* If broadcast address */ > if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) { > diff --git a/slirp/socket.c b/slirp/socket.c > index cb7b5b6..318301f 100644 > --- a/slirp/socket.c > +++ b/slirp/socket.c > @@ -701,10 +701,10 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr, > memset(&addr, 0, addrlen); > > DEBUG_CALL("tcp_listen"); > - DEBUG_ARG("haddr = %x", haddr); > - DEBUG_ARG("hport = %d", hport); > - DEBUG_ARG("laddr = %x", laddr); > - DEBUG_ARG("lport = %d", lport); > + DEBUG_ARG("haddr = %s", inet_ntoa((struct in_addr){.s_addr = haddr})); > + DEBUG_ARG("hport = %d", ntohs(hport)); > + DEBUG_ARG("laddr = %s", inet_ntoa((struct in_addr){.s_addr = laddr})); > + DEBUG_ARG("lport = %d", ntohs(lport)); > DEBUG_ARG("flags = %x", flags); > > so = socreate(slirp); > diff --git a/slirp/udp.c b/slirp/udp.c > index 227d779..e5bf065 100644 > --- a/slirp/udp.c > +++ b/slirp/udp.c > @@ -241,8 +241,8 @@ int udp_output(struct socket *so, struct mbuf *m, > DEBUG_CALL("udp_output"); > DEBUG_ARG("so = %p", so); > DEBUG_ARG("m = %p", m); > - DEBUG_ARG("saddr = %lx", (long)saddr->sin_addr.s_addr); > - DEBUG_ARG("daddr = %lx", (long)daddr->sin_addr.s_addr); > + DEBUG_ARG("saddr = %s", inet_ntoa(saddr->sin_addr)); > + DEBUG_ARG("daddr = %s", inet_ntoa(daddr->sin_addr)); > > /* > * Adjust for header > -- > 2.11.0 >
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180313044944.21058-1-aik@ozlabs.ru
Subject: [Qemu-devel] [PATCH qemu v2] slirp/debug: Print IP addresses in human readable form
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e91d6cceb5 slirp/debug: Print IP addresses in human readable form
=== OUTPUT BEGIN ===
Checking PATCH 1/1: slirp/debug: Print IP addresses in human readable form...
ERROR: code indent should never use tabs
#43: FILE: slirp/socket.c:704:
+^IDEBUG_ARG("haddr = %s", inet_ntoa((struct in_addr){.s_addr = haddr}));$
ERROR: code indent should never use tabs
#44: FILE: slirp/socket.c:705:
+^IDEBUG_ARG("hport = %d", ntohs(hport));$
ERROR: code indent should never use tabs
#45: FILE: slirp/socket.c:706:
+^IDEBUG_ARG("laddr = %s", inet_ntoa((struct in_addr){.s_addr = laddr}));$
ERROR: code indent should never use tabs
#46: FILE: slirp/socket.c:707:
+^IDEBUG_ARG("lport = %d", ntohs(lport));$
ERROR: code indent should never use tabs
#60: FILE: slirp/udp.c:244:
+^IDEBUG_ARG("saddr = %s", inet_ntoa(saddr->sin_addr));$
ERROR: code indent should never use tabs
#61: FILE: slirp/udp.c:245:
+^IDEBUG_ARG("daddr = %s", inet_ntoa(daddr->sin_addr));$
total: 6 errors, 0 warnings, 40 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On 03/12/2018 11:49 PM, Alexey Kardashevskiy wrote: > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > > checkpatch.pl complains on every single changed line as it keeps > using tabs - do I need to post 's/\t/ /g'? No. checkpatch.pl is guidance, but even stronger is 'be consistent to what you are editing'; we know to ignore the checkpatch warnings on slirp code as that has historically used tabs. Mixed style, where TABS occur in the context but your new additions use space, is also okay. (You CAN clean up the entire slirp files if you want, but get maintainer buy-in before doing so; and make the indentation cleanup separate from any other patch, so that a diff with whitespace ignored shows no change).
On 13/3/18 6:44 pm, Samuel Thibault wrote: > Alexey Kardashevskiy, on mar. 13 mars 2018 15:49:44 +1100, wrote: >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Applied to my tree, thanks! And what is your tree, is this something to be merged sometime later somewhere? :) > >> --- >> >> checkpatch.pl complains on every single changed line as it keeps >> using tabs - do I need to post 's/\t/ /g'? >> >> --- >> Changes: >> v2: >> * replaced static cast to (in_addr*) with temporary structs >> --- >> slirp/arp_table.c | 4 ++-- >> slirp/socket.c | 8 ++++---- >> slirp/udp.c | 4 ++-- >> 3 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/slirp/arp_table.c b/slirp/arp_table.c >> index 3547043..f81963b 100644 >> --- a/slirp/arp_table.c >> +++ b/slirp/arp_table.c >> @@ -33,7 +33,7 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t ethaddr[ETH_ALEN]) >> int i; >> >> DEBUG_CALL("arp_table_add"); >> - DEBUG_ARG("ip = 0x%x", ip_addr); >> + DEBUG_ARG("ip = %s", inet_ntoa((struct in_addr){.s_addr = ip_addr})); >> DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n", >> ethaddr[0], ethaddr[1], ethaddr[2], >> ethaddr[3], ethaddr[4], ethaddr[5])); >> @@ -67,7 +67,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr, >> int i; >> >> DEBUG_CALL("arp_table_search"); >> - DEBUG_ARG("ip = 0x%x", ip_addr); >> + DEBUG_ARG("ip = %s", inet_ntoa((struct in_addr){.s_addr = ip_addr})); >> >> /* If broadcast address */ >> if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) { >> diff --git a/slirp/socket.c b/slirp/socket.c >> index cb7b5b6..318301f 100644 >> --- a/slirp/socket.c >> +++ b/slirp/socket.c >> @@ -701,10 +701,10 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr, >> memset(&addr, 0, addrlen); >> >> DEBUG_CALL("tcp_listen"); >> - DEBUG_ARG("haddr = %x", haddr); >> - DEBUG_ARG("hport = %d", hport); >> - DEBUG_ARG("laddr = %x", laddr); >> - DEBUG_ARG("lport = %d", lport); >> + DEBUG_ARG("haddr = %s", inet_ntoa((struct in_addr){.s_addr = haddr})); >> + DEBUG_ARG("hport = %d", ntohs(hport)); >> + DEBUG_ARG("laddr = %s", inet_ntoa((struct in_addr){.s_addr = laddr})); >> + DEBUG_ARG("lport = %d", ntohs(lport)); >> DEBUG_ARG("flags = %x", flags); >> >> so = socreate(slirp); >> diff --git a/slirp/udp.c b/slirp/udp.c >> index 227d779..e5bf065 100644 >> --- a/slirp/udp.c >> +++ b/slirp/udp.c >> @@ -241,8 +241,8 @@ int udp_output(struct socket *so, struct mbuf *m, >> DEBUG_CALL("udp_output"); >> DEBUG_ARG("so = %p", so); >> DEBUG_ARG("m = %p", m); >> - DEBUG_ARG("saddr = %lx", (long)saddr->sin_addr.s_addr); >> - DEBUG_ARG("daddr = %lx", (long)daddr->sin_addr.s_addr); >> + DEBUG_ARG("saddr = %s", inet_ntoa(saddr->sin_addr)); >> + DEBUG_ARG("daddr = %s", inet_ntoa(daddr->sin_addr)); >> >> /* >> * Adjust for header >> -- >> 2.11.0 >> >
Alexey Kardashevskiy, le lun. 14 mai 2018 17:00:08 +1000, a ecrit: > On 13/3/18 6:44 pm, Samuel Thibault wrote: > > Alexey Kardashevskiy, on mar. 13 mars 2018 15:49:44 +1100, wrote: > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > Applied to my tree, thanks! > > > And what is your tree, is this something to be merged sometime later > somewhere? :) When I manage to take the time to, yes. Samuel
On 05/14/2018 02:00 AM, Alexey Kardashevskiy wrote: > On 13/3/18 6:44 pm, Samuel Thibault wrote: >> Alexey Kardashevskiy, on mar. 13 mars 2018 15:49:44 +1100, wrote: >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> Applied to my tree, thanks! > > > And what is your tree, is this something to be merged sometime later > somewhere? :) Per MAINTAINERS: SLIRP M: Samuel Thibault <samuel.thibault@ens-lyon.org> M: Jan Kiszka <jan.kiszka@siemens.com> S: Maintained F: slirp/ F: net/slirp.c F: include/net/slirp.h T: git git://git.kiszka.org/qemu.git queues/slirp which shows Jan's staging tree, but not Samuel's. It's not a requirement for a maintainer to have a public-facing staging tree, but many of them do, as it gives you a chance to test that what will later be in a pull request matches what you expect. At any rate, https://wiki.qemu.org/Contribute/SubmitAPatch#Is_my_patch_in.3F mentions that maintainers will batch together various related patches and send a pull request for inclusion in the main repository, thus even when a patch has been reviewed and staged, it may be another week or two before it lands in mainline. Yes, it can feel slow, but in general it works out for the best (as we have more chances to flag potential problems before they affect everyone by being in mainline).
On 16/5/18 12:30 am, Eric Blake wrote: > On 05/14/2018 02:00 AM, Alexey Kardashevskiy wrote: >> On 13/3/18 6:44 pm, Samuel Thibault wrote: >>> Alexey Kardashevskiy, on mar. 13 mars 2018 15:49:44 +1100, wrote: >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> >>> Applied to my tree, thanks! >> >> >> And what is your tree, is this something to be merged sometime later >> somewhere? :) > > Per MAINTAINERS: > > SLIRP > M: Samuel Thibault <samuel.thibault@ens-lyon.org> > M: Jan Kiszka <jan.kiszka@siemens.com> > S: Maintained > F: slirp/ > F: net/slirp.c > F: include/net/slirp.h > T: git git://git.kiszka.org/qemu.git queues/slirp > > which shows Jan's staging tree, but not Samuel's. > > It's not a requirement for a maintainer to have a public-facing staging > tree, but many of them do, as it gives you a chance to test that what will > later be in a pull request matches what you expect. > > At any rate, > https://wiki.qemu.org/Contribute/SubmitAPatch#Is_my_patch_in.3F mentions > that maintainers will batch together various related patches and send a > pull request for inclusion in the main repository, thus even when a patch > has been reviewed and staged, it may be another week or two before it lands > in mainline. Yes, it can feel slow, but in general it works out for the > best (as we have more chances to flag potential problems before they affect > everyone by being in mainline). If it was 2 weeks or even 6 - I would not bother but when it is 2 months, chances are it is forgotten/lost somewhere, hence my ping. I am not complaining at all, just pinging :)
diff --git a/slirp/arp_table.c b/slirp/arp_table.c index 3547043..f81963b 100644 --- a/slirp/arp_table.c +++ b/slirp/arp_table.c @@ -33,7 +33,7 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t ethaddr[ETH_ALEN]) int i; DEBUG_CALL("arp_table_add"); - DEBUG_ARG("ip = 0x%x", ip_addr); + DEBUG_ARG("ip = %s", inet_ntoa((struct in_addr){.s_addr = ip_addr})); DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n", ethaddr[0], ethaddr[1], ethaddr[2], ethaddr[3], ethaddr[4], ethaddr[5])); @@ -67,7 +67,7 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr, int i; DEBUG_CALL("arp_table_search"); - DEBUG_ARG("ip = 0x%x", ip_addr); + DEBUG_ARG("ip = %s", inet_ntoa((struct in_addr){.s_addr = ip_addr})); /* If broadcast address */ if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) { diff --git a/slirp/socket.c b/slirp/socket.c index cb7b5b6..318301f 100644 --- a/slirp/socket.c +++ b/slirp/socket.c @@ -701,10 +701,10 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr, memset(&addr, 0, addrlen); DEBUG_CALL("tcp_listen"); - DEBUG_ARG("haddr = %x", haddr); - DEBUG_ARG("hport = %d", hport); - DEBUG_ARG("laddr = %x", laddr); - DEBUG_ARG("lport = %d", lport); + DEBUG_ARG("haddr = %s", inet_ntoa((struct in_addr){.s_addr = haddr})); + DEBUG_ARG("hport = %d", ntohs(hport)); + DEBUG_ARG("laddr = %s", inet_ntoa((struct in_addr){.s_addr = laddr})); + DEBUG_ARG("lport = %d", ntohs(lport)); DEBUG_ARG("flags = %x", flags); so = socreate(slirp); diff --git a/slirp/udp.c b/slirp/udp.c index 227d779..e5bf065 100644 --- a/slirp/udp.c +++ b/slirp/udp.c @@ -241,8 +241,8 @@ int udp_output(struct socket *so, struct mbuf *m, DEBUG_CALL("udp_output"); DEBUG_ARG("so = %p", so); DEBUG_ARG("m = %p", m); - DEBUG_ARG("saddr = %lx", (long)saddr->sin_addr.s_addr); - DEBUG_ARG("daddr = %lx", (long)daddr->sin_addr.s_addr); + DEBUG_ARG("saddr = %s", inet_ntoa(saddr->sin_addr)); + DEBUG_ARG("daddr = %s", inet_ntoa(daddr->sin_addr)); /* * Adjust for header
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- checkpatch.pl complains on every single changed line as it keeps using tabs - do I need to post 's/\t/ /g'? --- Changes: v2: * replaced static cast to (in_addr*) with temporary structs --- slirp/arp_table.c | 4 ++-- slirp/socket.c | 8 ++++---- slirp/udp.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-)