Re: [Qemu-devel] [PATCH] block/iscsi: bump libiscsi requirement to 1.8.0

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/06/2014 14:33, Peter Lieven ha scritto:
> This patch lifts the minimum supported libiscsi version from 1.4.0 to
> 1.8.0 in preparation to an upcoming patch to dynamically switch between
> 10 Byte and 16 Byte CDBs.
>
> On one this allows us to remove nearly all #ifdefs from the code which
> makes the code easier to maintain and read. On the other hand
> I would not recommend libiscsi prior to 1.8.0 for production use
> because the following important libiscsi fixes for deadlocks and
> protocol errors are missing prior to 1.8.0:
>
> dbe9a1e SOCKET queue cmd PDUs directly in waitpdu queue
> 30df192 DATA-OUT set pdu->cmdsn appropriately
> 548bd22 ISCSI fix broken send logic in iscsi_scsi_async_command
> 14bee10 RECONNECT do not increase CmdSN for immediate PDUs
> 1f4a66a PDU queue out PDUs in order of itt.
> 562dd46 PDU avoid incrementing itt to 0xffffffff
> cd09c0f PDU use serial32 arithmetic for cmdsn, maxcmdsn and expcmdsn.
> 89e918e SOCKET validate data_size in in_pdu header
> 91267f5 Limit immediate and unsolicited data to FirstBurstLength
>
> Note that libiscsi 1.8.0 was already released Jan 5th, 2013.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |   66 ---------------------------------------------------------
>  configure     |   39 +++-------------------------------
>  2 files changed, 3 insertions(+), 102 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 1bf9ccd..7394fa6 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -366,17 +366,6 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,
>
>      lba = sector_qemu2lun(sector_num, iscsilun);
>      num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
> -#if !defined(LIBISCSI_FEATURE_IOVECTOR)
> -    /* if the iovec only contains one buffer we can pass it directly */
> -    if (iov->niov == 1) {
> -        data = iov->iov[0].iov_base;
> -    } else {
> -        size_t size = MIN(nb_sectors * BDRV_SECTOR_SIZE, iov->size);
> -        buf = g_malloc(size);
> -        qemu_iovec_to_buf(iov, 0, buf, size);
> -        data = buf;
> -    }
> -#endif
>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>  retry:
>      iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
> @@ -387,10 +376,8 @@ retry:
>          g_free(buf);
>          return -ENOMEM;
>      }
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>      scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
>                            iov->niov);
> -#endif
>      while (!iTask.complete) {
>          iscsi_set_events(iscsilun);
>          qemu_coroutine_yield();
> @@ -417,7 +404,6 @@ retry:
>      return 0;
>  }
>
> -
>  static bool iscsi_allocationmap_is_allocated(IscsiLun *iscsilun,
>                                               int64_t sector_num, int nb_sectors)
>  {
> @@ -430,9 +416,6 @@ static bool iscsi_allocationmap_is_allocated(IscsiLun *iscsilun,
>                             sector_num / iscsilun->cluster_sectors) == size);
>  }
>
> -
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
> -
>  static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs,
>                                                    int64_t sector_num,
>                                                    int nb_sectors, int *pnum)
> @@ -530,9 +513,6 @@ out:
>      return ret;
>  }
>
> -#endif /* LIBISCSI_FEATURE_IOVECTOR */
> -
> -
>  static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>                                         int64_t sector_num, int nb_sectors,
>                                         QEMUIOVector *iov)
> @@ -541,15 +521,11 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>      struct IscsiTask iTask;
>      uint64_t lba;
>      uint32_t num_sectors;
> -#if !defined(LIBISCSI_FEATURE_IOVECTOR)
> -    int i;
> -#endif
>
>      if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
>          return -EINVAL;
>      }
>
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>      if (iscsilun->lbprz && nb_sectors >= ISCSI_CHECKALLOC_THRES &&
>          !iscsi_allocationmap_is_allocated(iscsilun, sector_num, nb_sectors)) {
>          int64_t ret;
> @@ -563,7 +539,6 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,
>              return 0;
>          }
>      }
> -#endif
>
>      lba = sector_qemu2lun(sector_num, iscsilun);
>      num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
> @@ -581,24 +556,14 @@ retry:
>          iTask.task = iscsi_read10_task(iscsilun->iscsi, iscsilun->lun, lba,
>                                         num_sectors * iscsilun->block_size,
>                                         iscsilun->block_size,
> -#if !defined(CONFIG_LIBISCSI_1_4) /* API change from 1.4.0 to 1.5.0 */
>                                         0, 0, 0, 0, 0,
> -#endif
>                                         iscsi_co_generic_cb, &iTask);
>          break;
>      }
>      if (iTask.task == NULL) {
>          return -ENOMEM;
>      }
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>      scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, iov->niov);
> -#else
> -    for (i = 0; i < iov->niov; i++) {
> -        scsi_task_add_data_in_buffer(iTask.task,
> -                                     iov->iov[i].iov_len,
> -                                     iov->iov[i].iov_base);
> -    }
> -#endif
>
>      while (!iTask.complete) {
>          iscsi_set_events(iscsilun);
> @@ -753,18 +718,9 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>              data.data = acb->ioh->dxferp;
>              data.size = acb->ioh->dxfer_len;
>          } else {
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>              scsi_task_set_iov_out(acb->task,
>                                   (struct scsi_iovec *) acb->ioh->dxferp,
>                                   acb->ioh->iovec_count);
> -#else
> -            struct iovec *iov = (struct iovec *)acb->ioh->dxferp;
> -
> -            acb->buf = g_malloc(acb->ioh->dxfer_len);
> -            data.data = acb->buf;
> -            data.size = iov_to_buf(iov, acb->ioh->iovec_count, 0,
> -                                   acb->buf, acb->ioh->dxfer_len);
> -#endif
>          }
>      }
>
> @@ -784,20 +740,9 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>                                           acb->ioh->dxfer_len,
>                                           acb->ioh->dxferp);
>          } else {
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>              scsi_task_set_iov_in(acb->task,
>                                   (struct scsi_iovec *) acb->ioh->dxferp,
>                                   acb->ioh->iovec_count);
> -#else
> -            int i;
> -            for (i = 0; i < acb->ioh->iovec_count; i++) {
> -                struct iovec *iov = (struct iovec *)acb->ioh->dxferp;
> -
> -                scsi_task_add_data_in_buffer(acb->task,
> -                    iov[i].iov_len,
> -                    iov[i].iov_base);
> -            }
> -#endif
>          }
>      }
>
> @@ -806,7 +751,6 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>      return &acb->common;
>  }
>
> -
>  static void ioctl_cb(void *opaque, int status)
>  {
>      int *p_status = opaque;
> @@ -912,7 +856,6 @@ retry:
>  }
>
>  #if defined(SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED)
> -
>  static int
>  coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
>                                     int nb_sectors, BdrvRequestFlags flags)
> @@ -989,7 +932,6 @@ retry:
>
>      return 0;
>  }
> -
>  #endif /* SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED */
>
>  static void parse_chap(struct iscsi_context *iscsi, const char *target,
> @@ -1101,7 +1043,6 @@ static char *parse_initiator_name(const char *target)
>      return iscsi_name;
>  }
>
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
>  static void iscsi_nop_timed_event(void *opaque)
>  {
>      IscsiLun *iscsilun = opaque;
> @@ -1119,7 +1060,6 @@ static void iscsi_nop_timed_event(void *opaque)
>      timer_mod(iscsilun->nop_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
>      iscsi_set_events(iscsilun);
>  }
> -#endif
>
>  static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp)
>  {
> @@ -1258,14 +1198,12 @@ static void iscsi_attach_aio_context(BlockDriverState *bs,
>      iscsilun->aio_context = new_context;
>      iscsi_set_events(iscsilun);
>
> -#if defined(LIBISCSI_FEATURE_NOP_COUNTER)
>      /* Set up a timer for sending out iSCSI NOPs */
>      iscsilun->nop_timer = aio_timer_new(iscsilun->aio_context,
>                                          QEMU_CLOCK_REALTIME, SCALE_MS,
>                                          iscsi_nop_timed_event, iscsilun);
>      timer_mod(iscsilun->nop_timer,
>                qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
> -#endif
>  }
>
>  /*
> @@ -1457,13 +1395,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) {
>          iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran *
>                                       iscsilun->block_size) >> BDRV_SECTOR_BITS;
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>          if (iscsilun->lbprz && !(bs->open_flags & BDRV_O_NOCACHE)) {
>              iscsilun->allocationmap =
>                  bitmap_new(DIV_ROUND_UP(bs->total_sectors,
>                                          iscsilun->cluster_sectors));
>          }
> -#endif
>      }
>
>  out:
> @@ -1652,9 +1588,7 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_truncate   = iscsi_truncate,
>      .bdrv_refresh_limits = iscsi_refresh_limits,
>
> -#if defined(LIBISCSI_FEATURE_IOVECTOR)
>      .bdrv_co_get_block_status = iscsi_co_get_block_status,
> -#endif
>      .bdrv_co_discard      = iscsi_co_discard,
>  #if defined(SCSI_SENSE_ASCQ_CAPACITY_DATA_HAS_CHANGED)
>      .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
> diff --git a/configure b/configure
> index 69b9f56..1ef998e 100755
> --- a/configure
> +++ b/configure
> @@ -3346,46 +3346,20 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> -# Do we have libiscsi
> -# We check for iscsi_write16_sync() to make sure we have a
> -# at least version 1.4.0 of libiscsi.
> +# Do we have libiscsi >= 1.8.0
>  if test "$libiscsi" != "no" ; then
> -  cat > $TMPC << EOF
> -#include <stdio.h>
> -#include <iscsi/iscsi.h>
> -int main(void) { iscsi_write16_sync(NULL,0,0,NULL,0,0,0,0,0,0,0); return 0; }
> -EOF
> -  if $pkg_config --atleast-version=1.7.0 libiscsi; then
> +  if $pkg_config --atleast-version=1.8.0 libiscsi; then
>      libiscsi="yes"
>      libiscsi_cflags=$($pkg_config --cflags libiscsi)
>      libiscsi_libs=$($pkg_config --libs libiscsi)
> -  elif compile_prog "" "-liscsi" ; then
> -    libiscsi="yes"
> -    libiscsi_libs="-liscsi"
>    else
>      if test "$libiscsi" = "yes" ; then
> -      feature_not_found "libiscsi" "Install libiscsi devel"
> +      feature_not_found "libiscsi" "Install libiscsi >= 1.8.0"
>      fi
>      libiscsi="no"
>    fi
>  fi
>
> -# We also need to know the API version because there was an
> -# API change from 1.4.0 to 1.5.0.
> -if test "$libiscsi" = "yes"; then
> -  cat >$TMPC <<EOF
> -#include <iscsi/iscsi.h>
> -int main(void)
> -{
> -  iscsi_read10_task(0, 0, 0, 0, 0, 0, 0);
> -  return 0;
> -}
> -EOF
> -  if compile_prog "" "-liscsi"; then
> -    libiscsi_version="1.4.0"
> -  fi
> -fi
> -
>  ##########################################
>  # Do we need libm
>  cat > $TMPC << EOF
> @@ -4160,11 +4134,7 @@ echo "nss used          $smartcard_nss"
>  echo "libusb            $libusb"
>  echo "usb net redir     $usb_redir"
>  echo "GLX support       $glx"
> -if test "$libiscsi_version" = "1.4.0"; then
> -echo "libiscsi support  $libiscsi (1.4.0)"
> -else
>  echo "libiscsi support  $libiscsi"
> -fi
>  echo "libnfs support    $libnfs"
>  echo "build guest agent $guest_agent"
>  echo "QGA VSS support   $guest_agent_with_vss"
> @@ -4519,9 +4489,6 @@ fi
>
>  if test "$libiscsi" = "yes" ; then
>    echo "CONFIG_LIBISCSI=m" >> $config_host_mak
> -  if test "$libiscsi_version" = "1.4.0"; then
> -    echo "CONFIG_LIBISCSI_1_4=y" >> $config_host_mak
> -  fi
>    echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
>    echo "LIBISCSI_LIBS=$libiscsi_libs" >> $config_host_mak
>  fi
>

Applied now, with further simplifications to bump to 1.9.0.

Paolo