On Thu, Mar 18, 2010 at 04:13:05AM +0700, Neutron Soutmun wrote:
> Package: nbd
> Version: 2.9.14-3
> Severity: normal
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Tue, Mar 16, 2010 at 01:24:54PM +0100, Wouter Verhelst wrote:
> > On Tue, Mar 16, 2010 at 06:36:22PM +0700, Neutron Soutmun wrote:
> > I should probably point out that there's a git tree for the upstream
> > code: it's on github ("/yoe/nbd"), and on sourceforge
> > (nbd.git.sourceforge.net).
>
> $ git clone http://github.com/yoe/nbd.git
>
> now, follow up but keep working continue on the old patch which won't get me
> more confusion.
Well, part of the reason I pointed you to that repository is the fact
that there's a patch in there (for #557810) that conflicts with yours.
It'd be cool if you could update it so that it would apply cleanly to
that version, and then I can easily do an upstream release immediately
followed by a Debian package.
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> I have provide the initial patch for IPv6 support in
> >> nbd-{server, client} which the nbd-server able to
> >> serve in both IPv4 and IPv6 requests.
> >>
> >> Patch is attached.
> >>
> >> The client can connect to the server that listen on
> >> IPv6 or IPv4 like this on command line
> >>
> >> # nbd-client ::1...@11111 /dev/nbd0
> >> or
> >> # nbd-client 127.0....@11111 /dev/nbd0
> >>
> >> Substitute the port separator from ":" to "@".
> >
> > Indeed, that would be required for proper IPv6 support.
> >
> > However, I'd like to have some way of backwards compatibility. Since
> > IPv6 addresses have at least two colons in them, we could check whether
> > there are two or more, and if not, assume that the colon is still the
> > separator.
> >
> > That shouldn't cause too much of a problem.
>
> Update as your advice which now the IPv4 address and port could either have
> separator ":" or "@" but the IPv6 is strictly "@".
Good.
> >> /**
> >> + * duplicate server
> >> + * @param s the old server we want to duplicate
> >> + **/
> >> +SERVER* dup_serve(SERVER *s) {
> >> + SERVER *serve = NULL;
> >> +
> >> + serve=g_new0(SERVER, 1);
> >> + if (serve == NULL)
> >> + return NULL;
> >> +
> >> + memset(serve,0,sizeof(SERVER));
> >
> > g_new0 already does this (that's the 0 in the function name)
>
> Fixed. (Now I possible to read more on devhelp, thanks Theppitak)
>
> >> @@ -690,7 +736,17 @@
> >> if(i>0) {
> >> if(!s.listenaddr) {
> >> s.listenaddr = g_strdup("0.0.0.0");
> >> + s.socket_family = AF_UNSPEC;
> >> +
> >> + ns = dup_serve (&s);
> >> + if (ns) {
> >> + ns->socket_family = AF_INET6;
> >> + g_array_append_val(retval, *ns);
> >> + free(ns);
> >> + ns = NULL;
> >> + }
> >
> >What is the reason for that? Doesn't AF_UNSPEC already mean we're
> >listening for v6 addresses, too? Or am I missing something?
>
> nbd-server using SERVER that only has a single listening socket which
> if I need to handle both IPv4 (AF_INET) and IPv6 (AF_INET6), I must
> setup both in separated server (IPv6 could handle the IPv4 connection
> but only in the system that allow to do the v4 mapped).
Ah, right, I forgot about that bit. Yes, indeed, v6-mapped-v4 is no
longer the default, for some strange reason.
> The AF_UNSPEC is only using for the hinting to calling getaddrinfo()
> which it may return the addresses (linked-list) of IPv4 and/or IPv6.
> We could use these addrinfo data to setup the listening socket and
> also bind it.
Okay.
> >> - if (getpeername(net, (struct sockaddr *) &addrin, (socklen_t
> >> *)&addrinlen) < 0)
> >> + if (getpeername(net, (struct sockaddr *)&addrin, (socklen_t
> >> *)&addrinlen) < 0)
> >
> > Please don't do gratuitous whitespace changes; they convolute the patch
> > and make it harder to see what's going on.
>
> Sorry, just confuse a litle bit to follow your code-style but I try to keep it
> as you want.
Indeed. Code style cleanups are certainly welcome, but it's preferable
if they're separate from functional patches.
[...]
> > This would mean that if someone were to use 'nbd-server 0.0.0.0:1234' to
> > explicitly bind to an IPv4 address, this would not work.
> >
> > I think the disabling of IPv6 for those who want it, should still be
> > possible.
>
> Aha, it must possible to do so.
> I fixed it in the new patch and I found that the last time patch only
> handle the parsing config file to setup the server(s) but not for the
> parsing cmdline which if I parsing like this
>
> # nbd-server 12345 /path/to/file.img
>
> It only bind to AF_INET or AF_INET6 depending on which one return first
> but not both.
Right, that's not good :)
> Therefore, the new patch has added more patching on the code to
> manipulate the serve that return from the cmdline() parsing function.
Great.
[...]
> > The rest of the patch looks fine, on first glance.
> >
> > If you could update it with those comments (I'm not sure I understand
> > all the intricacies yet), I'll push the patch upstream and do an upload
> > ASAP.
>
> Thanks for your instantly reply, today just go outside for sites seeing.
> It's the schedule of Thailand MiniDebCamp 2010 and after come back to the
> resident and have a time to review your comments and hands-on the code,
> finally I have new patch and please review again.
[...]
No further comments on that one. If you can update to the 'master'
branch in git, and my tests reveal that it works properly, I'll do an
upstream release and Debian upload ASAP.
Thanks!
--
The biometric identification system at the gates of the CIA headquarters
works because there's a guard with a large gun making sure no one is
trying to fool the system.
http://www.schneier.com/blog/archives/2009/01/biometrics.html
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
