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
