Re: [Qemu-devel] [PATCH 02/14] ./block/iscsi/socket.c

[Stefan Hajnoczi]

On Fri, Dec 3, 2010 at 11:09 AM,  <address@hidden> wrote:
> +int
> +iscsi_connect_async(struct iscsi_context *iscsi, const char *portal,
> +                   iscsi_command_cb cb, void *private_data)
> +{
> +       int tpgt = -1;
> +       int port = 3260;
> +       char *str;
> +       char *addr;
> +       struct sockaddr_storage s;
> +       struct sockaddr_in *sin = (struct sockaddr_in *)&s;
> +       int socksize;
> +
> +       if (iscsi->fd != -1) {
> +               iscsi_set_error(iscsi,
> +                               "Trying to connect but already connected.");
> +               return -1;
> +       }
> +
> +       addr = strdup(portal);
> +       if (addr == NULL) {
> +               iscsi_set_error(iscsi, "Out-of-memory: "
> +                               "Failed to strdup portal address.");
> +               return -2;

Please use constants.  -1, -2, ... don't tell me much and are
undocumented.  I think your strategy is a unique code for every error
return inside a function?

> +       }
> +
> +       /* check if we have a target portal group tag */
> +       str = rindex(addr, ',');
> +       if (str != NULL) {
> +               tpgt = atoi(str+1);
> +               str[0] = 0;
> +       }
> +
> +       /* XXX need handling for {ipv6 addresses} */
> +       /* for now, assume all is ipv4 */
> +       str = rindex(addr, ':');
> +       if (str != NULL) {
> +               port = atoi(str+1);
> +               str[0] = 0;
> +       }
> +
> +       sin->sin_family = AF_INET;
> +       sin->sin_port   = htons(port);
> +       if (inet_pton(AF_INET, addr, &sin->sin_addr) != 1) {
> +               iscsi_set_error(iscsi, "Invalid target:%s  "
> +                               "Failed to convert to ip address.", addr);
> +               free(addr);
> +               return -3;
> +       }
> +       free(addr);
> +
> +       switch (s.ss_family) {
> +       case AF_INET:
> +               iscsi->fd = socket(AF_INET, SOCK_STREAM, 0);
> +               socksize = sizeof(struct sockaddr_in);
> +               break;
> +       default:
> +               iscsi_set_error(iscsi, "Unknown address family :%d. "
> +                               "Only IPv4 supported so far.", s.ss_family);
> +               return -4;
> +
> +       }
> +
> +       if (iscsi->fd == -1) {
> +               iscsi_set_error(iscsi, "Failed to open iscsi socket. "
> +                               "Errno:%s(%d).", strerror(errno), errno);
> +               return -5;
> +
> +       }
> +
> +       iscsi->connect_cb  = cb;
> +       iscsi->connect_data = private_data;
> +
> +       set_nonblocking(iscsi->fd);
> +
> +       if (connect(iscsi->fd, (struct sockaddr *)&s, socksize) != 0
> +           && errno != EINPROGRESS) {
> +               iscsi_set_error(iscsi, "Connect failed with errno : "
> +                               "%s(%d)\n", strerror(errno), errno);

Since the function will return an error we should close(iscsi->fd) and
set it to -1 again.

> +               return -6;
> +       }
> +
> +       return 0;
> +}

> +static int
> +iscsi_read_from_socket(struct iscsi_context *iscsi)
> +{
> +       int available;
> +       int size;
> +       unsigned char *buf;
> +       ssize_t count;
> +
> +       if (ioctl(iscsi->fd, FIONREAD, &available) != 0) {
> +               iscsi_set_error(iscsi, "ioctl FIONREAD returned error : "
> +                               "%d\n", errno);

iscsi_set_error() newlines are used inconsistently.  Some calls
include the newline, some don't.

> +               return -1;
> +       }
> +       if (available == 0) {
> +               iscsi_set_error(iscsi, "no data readable in socket, "
> +                               "socket is closed\n");
> +               return -2;
> +       }
> +       size = iscsi->insize - iscsi->inpos + available;
> +       buf = malloc(size);
> +       if (buf == NULL) {
> +               iscsi_set_error(iscsi, "failed to allocate %d bytes for "
> +                               "input buffer\n", size);
> +               return -3;
> +       }
> +       if (iscsi->insize > iscsi->inpos) {
> +               memcpy(buf, iscsi->inbuf + iscsi->inpos,
> +                      iscsi->insize - iscsi->inpos);
> +               iscsi->insize -= iscsi->inpos;
> +               iscsi->inpos   = 0;
> +       }
> +
> +       count = read(iscsi->fd, buf + iscsi->insize, available);
> +       if (count == -1) {
> +               if (errno == EINTR) {
> +                       free(buf);
> +                       buf = NULL;
> +                       return 0;

If iscsi->insize > iscsi->inpos was true above then we've made insize,
inpos, inbuf inconsistent by returning early here.

> +               }
> +               iscsi_set_error(iscsi, "read from socket failed, "
> +                               "errno:%d\n", errno);
> +               free(buf);
> +               buf = NULL;
> +               return -4;

Same issue here.

> +       }
> +
> +       if (iscsi->inbuf != NULL) {
> +               free(iscsi->inbuf);
> +       }
> +       iscsi->inbuf   = buf;
> +       iscsi->insize += count;
> +
> +       while (1) {
> +               if (iscsi->insize - iscsi->inpos < 48) {

Please use ISCSI_(RAW_)HEADER_SIZE instead of 48.

> +                       return 0;
> +               }
> +               count = iscsi_get_pdu_size(iscsi,
> +                                          iscsi->inbuf + iscsi->inpos);
> +               if (iscsi->insize + iscsi->inpos < count) {

I don't follow this check.  Should it be:
if (iscsi->insize - iscsi->inpos < count) { /* or maybe
ISCSI_HEADER_SIZE + count */

> +                       return 0;
> +               }
> +               if (iscsi_process_pdu(iscsi, iscsi->inbuf + iscsi->inpos,
> +                                     count) != 0) {
> +                       free(buf);

Freeing buf is dangerous here since iscsi->inbuf == buf.

> +                       return -7;
> +               }
> +               iscsi->inpos += count;
> +               if (iscsi->inpos == iscsi->insize) {
> +                       free(iscsi->inbuf);
> +                       iscsi->inbuf = NULL;
> +                       iscsi->insize = 0;
> +                       iscsi->inpos = 0;
> +               }
> +               if (iscsi->inpos > iscsi->insize) {
> +                       iscsi_set_error(iscsi, "inpos > insize. bug!\n");
> +                       return -6;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int
> +iscsi_write_to_socket(struct iscsi_context *iscsi)
> +{
> +       ssize_t count;
> +
> +       if (iscsi->fd == -1) {
> +               iscsi_set_error(iscsi, "trying to write but not connected\n");
> +               return -2;
> +       }
> +
> +       while (iscsi->outqueue != NULL) {
> +               ssize_t total;
> +
> +               total = iscsi->outqueue->outdata.size;
> +               total = (total + 3) & 0xfffffffc;
> +
> +               count = write(iscsi->fd,
> +                             iscsi->outqueue->outdata.data
> +                             + iscsi->outqueue->written,
> +                             total - iscsi->outqueue->written);

The assumption is that iscsi->outqueue->outdata.data[] is sized to a
multiple of 4.  I haven't read enough code later in the series yet to
know whether the assumption is always true.

Is it possible to move the assumption higher up into the pdu code
instead of hardcoding it here?

> +               if (count == -1) {
> +                       if (errno == EAGAIN || errno == EWOULDBLOCK) {
> +                               return 0;
> +                       }
> +                       iscsi_set_error(iscsi, "Error when writing to "
> +                                       "socket :%d\n", errno);
> +                       return -3;
> +               }
> +
> +               iscsi->outqueue->written += count;
> +               if (iscsi->outqueue->written == total) {
> +                       struct iscsi_pdu *pdu = iscsi->outqueue;
> +
> +                       SLIST_REMOVE(&iscsi->outqueue, pdu);
> +                       SLIST_ADD_END(&iscsi->waitpdu, pdu);
> +               }
> +       }
> +       return 0;
> +}
> +
> +int
> +iscsi_service(struct iscsi_context *iscsi, int revents)
> +{
> +       if (revents & POLLERR) {
> +               iscsi_set_error(iscsi, "iscsi_service: POLLERR, "
> +                               "socket error.");
> +               iscsi->connect_cb(iscsi, SCSI_STATUS_ERROR, NULL,
> +                                 iscsi->connect_data);

connect_cb is invoked on any socket error, including after the
connection has been established.  Perhaps a better name is just
"callback" or "state_changed" or "status_cb" because "connect_cb"
implies this callback is only used for the connect operation.

> +               return -1;
> +       }
> +       if (revents & POLLHUP) {
> +               iscsi_set_error(iscsi, "iscsi_service: POLLHUP, "
> +                               "socket error.");
> +               iscsi->connect_cb(iscsi, SCSI_STATUS_ERROR, NULL,
> +                                 iscsi->connect_data);
> +               return -2;
> +       }
> +
> +       if (iscsi->is_connected == 0 && iscsi->fd != -1 && revents&POLLOUT) {
> +               iscsi->is_connected = 1;
> +               iscsi->connect_cb(iscsi, SCSI_STATUS_GOOD, NULL,
> +                                 iscsi->connect_data);
> +               return 0;
> +       }
> +
> +       if (revents & POLLOUT && iscsi->outqueue != NULL) {
> +               if (iscsi_write_to_socket(iscsi) != 0) {
> +                       return -3;

You choose not to propagate the specific error return value from
iscsi_write_to_socket()?

> +               }
> +       }
> +       if (revents & POLLIN) {
> +               if (iscsi_read_from_socket(iscsi) != 0)
> +                       return -4;

...same here.

> +       }
> +
> +       return 0;
> +}
> +
> +int
> +iscsi_queue_pdu(struct iscsi_context *iscsi, struct iscsi_pdu *pdu)
> +{
> +       if (pdu == NULL) {
> +               iscsi_set_error(iscsi, "trying to queue NULL pdu");
> +               return -2;
> +       }
> +
> +       if (iscsi->header_digest != ISCSI_HEADER_DIGEST_NONE) {
> +               unsigned long crc;
> +
> +               crc = crc32c((char *)pdu->outdata.data, 
> ISCSI_RAW_HEADER_SIZE);

crc32c() hasn't been posted yet, it is later in the patch series.
Self-contained patches would be easier to review.  Later patches can
always add more things to the same files.

Just a general comment, I'm not asking that you resend.

> +
> +               pdu->outdata.data[ISCSI_RAW_HEADER_SIZE+3] = (crc >> 24)&0xff;
> +               pdu->outdata.data[ISCSI_RAW_HEADER_SIZE+2] = (crc >> 16)&0xff;
> +               pdu->outdata.data[ISCSI_RAW_HEADER_SIZE+1] = (crc >>  8)&0xff;
> +               pdu->outdata.data[ISCSI_RAW_HEADER_SIZE+0] = (crc)      &0xff;

Is a size check on outdata needed to make sure there is enough space
for the CRC?  Normally there should be enough space, but just for
robustness it makes sense to error out on a short pdu.

Stefan