Re: [Qemu-devel] [PATCH] sheepdog: add support for connecting to unix domain socket

[MORITA Kazutaka <morita.kazutaka@gmail.com>]

At Wed, 16 Jan 2013 15:21:09 +0100,
Stefan Hajnoczi wrote:
> 
> On Tue, Jan 15, 2013 at 11:12:40PM +0900, MORITA Kazutaka wrote:
> > +static int set_nodelay(int fd)
> > +{
> > +    int opt = 1;
> > +    return setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&opt, sizeof(opt));
> > +}
> > +
> 
> Please split this into a separate patch that moves the function to
> util/osdep.c and names it socket_set_nodelay().  You can put it below
> socket_set_cork().

Agreed.

> 
> > @@ -804,23 +781,14 @@ static int set_nodelay(int fd)
> >   */
> >  static int get_sheep_fd(BDRVSheepdogState *s)
> >  {
> > -    int ret, fd;
> > +    int fd;
> >  
> > -    fd = connect_to_sdog(s->addr, s->port);
> > +    fd = connect_to_sdog(s->host_spec);
> 
> The patch would be easier to review if you split out a separate patch to
> move from addr/port to host_spec.  Then the final patch can focus just
> on UNIX domain sockets without all the addr/port to host_spec changes.

Agreed.

> 
> >      if (fd < 0) {
> >          error_report("%s", strerror(errno));
> 
> connect_to_sdog() does not set errno and it already reports errors
> internally.  Can this error_report() be dropped?

Yes, I'll remove it.

> 
> >          return fd;
> >      }
> >  
> > -    socket_set_nonblock(fd);
> 
> Where is nonblock being set now that you've removed this?

Oops, I thought that unix_connect and inet_connect return nonblocking
sockets, but it was wrong.

> >  
> > -``sheepdog:<host>:<port>:<vdiname>:<tag>''
> > +using Unix Domain Socket:
> > +@example
> > +sheepdog:unix:<domain-socket>:<vdiname>[:<snapid or tag>]
> > +@end example
> 
> Please document that <domain-socket> must be an absolute path.  This
> will prevent confusing users who try a relative path.

Okay.

Thanks for your comments, I'll send v2.

Kazutaka