On Wed, Aug 27, 2008 at 05:09:52PM +0200, Julius Volz wrote:
> On Wed, Aug 27, 2008 at 9:17 AM, Simon Horman wrote:
> > Here is a second round of thoughts after having gone through the whole series.
>
> Thanks for wading through this!
>
> > [PATCH RFC 06/24] IPVS: Add debug macros for v4 and v6 address output
> >
> > * The #defines in ip_vs_dbg_addr seem a bit aquard.
> > Could it be rearanged liks this?
> >
> > static inline const char *ip_vs_dbg_addr(int af, char *buf, size_t buf_len,
> > const union nf_inet_addr *addr,
> > int *idx)
> > {
> > int len;
> > #ifdef CONFIG_IP_VS_IPV6
> > if (af == AF_INET6)
> > len = snprintf(&buf[*idx], buf_len - *idx, "[" NIP6_FMT "]",
> > NIP6(addr->in6)) + 1;
> > else
> > #endif
> > len = snprintf(&buf[*idx], buf_len - *idx, NIPQUAD_FMT,
> > NIPQUAD(addr->ip)) + 1;
> >
> > *idx += len;
> > return &buf[*idx - len];
> > }
>
> Yes, that looks nicer and more like the other cases!
>
> > * The comment "/* Only use from within IP_VS_DBG_BUF() macro */"
> > should also mention usage inside IP_VS_ERR_BUF()
>
> Right, thanks!
>
> > * If IP_VS_DBG_ADDR() is used more than once inside a single
> > IP_VS_DBG_BUF() or IP_VS_ERR_BUF() call, won't ip_vs_dbg_buf
> > be set to the value one of the calls to IP_VS_DBG_ADDR,
> > thus overwriting other calls and producing incorrect debugging
> > output?
>
> No, the buffer can receive several strings (but it's limited in size,
> so you have to be careful). An index to the current position in the
> buffer is maintained in the 'idx' variable between multiple
> IP_VS_DBG_ADDR calls used in the same outer macro.
>
> In general, I'm a bit unsure if this kind of macro magic is acceptable
> style, but it was the best way I could come up with to merge
> alternating v4 and v6 output without too much code duplication.
Thanks, I knew I was missing something obvious. From a style point of
view, I'm not sure either, but it seems like a reasonable start. Perhaps a
slight enhancement would be to add a BUG_ON() to ip_vs_dbg_addr() which will
trigger if ip_vs_dbg_buf will overflow.
> > [PATCH RFC 15/24] IPVS: Add support for IPv6 entry output in procfs files
> >
> > * The netlink-aware ipvsadm code also seems to allow for dotted-quad
> > representation of ipv4 addresses in proc. Is that representation used
> > or planned to be used?
>
> Good catch! No, I wasn't even aware of that feature in the new ipvsadm
> (but now I see it). I think it should be removed because it is
> effectively dead code (the existing v4 proc format shouldn't be
> changed). Vince, do you agree?
I only noticed it from fixing up the atio() problem in ipvsadm
that I posted a patch for the other day. Its not a big deal. But
it would be nice to eliminate dead code.
> > [PATCH RFC 17/24] IPVS: Make proc/net files output IPv6 entries
> >
> > * It might be cleaner to do:
> >
> > #ifdef CONFIG_IP_VS_IPV6
> > if (cp->af == AF_INET6)
> > seq_printf ...
> > else
> > #endif
> > seq_printf ...
>
> Yes, it's nicer this way around, I'll change that!
>
> > General
> >
> > * You need to reorder and or merge patches such that after each
> > patch is applied the code will build and run. It is ok for
> > a patch to add code which isn't used until a later patch is applied.
>
> Yes, I have found no nice way to achieve this yet :( At least not when
> reworking the complete end result (one big patch) into smaller
> patches, because there is so much interdependency and several logical
> changes within the same hunks (or even lines). I might have to
> manually do a step-by-step adding of the logical code features to get
> this... I will try to work on that next.
I realise this is a pain but unfortunately it is needed.
> > * Where possible please make lines <= 80 columns wide
>
> Yes, I will check for that more strictly now (unless it really looks
> nicer otherwise), thanks!
This is less critical, but thankfully easy :-)
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
