Re: [PATCH RESEND v3 4/8] meson: move iscsiadm check into storage_iscsi condition

[Andrea Bolognani]

On Tue, May 25, 2021 at 02:36:04AM -0700, Andrea Bolognani wrote:
> On Tue, May 25, 2021 at 10:00:43AM +0200, Pavel Hrdina wrote:
> > +  if not get_option('storage_iscsi').disabled()
> > +iscsi_enable = true
> > +iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
> > libvirt_sbin_path)
> > +
> > +if not iscsiadm_prog.found()
> > +  if get_option('storage_iscsi').enabled()
> > +error('We need iscsiadm for iSCSI storage driver')
> > +  else
> > +iscsi_enable = false
> > +  endif
> > +endif
>
> I believe this could be rewritten as
>
>   if not get_option('storage_iscsi').disabled()
> iscsi_enable = true
> iscsiadm_prog = find_program('iscsiadm', required: 
> get_option('storage_iscsi'), dirs: libvirt_sbin_path)
>
> if not iscsiadm_prog.found()
>   iscsi_enable = false
> endif
>
> I see this pattern used elsewhere in the file and it's more concise
> while achieving the same result AFAICT.

I just realized that you rewrite this check again in 6/8, so no
matter what it looks like in this commit

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v3 4/8] meson: move iscsiadm check into storage_iscsi condition

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:43AM +0200, Pavel Hrdina wrote:
> +  if not get_option('storage_iscsi').disabled()
> +iscsi_enable = true
> +iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
> libvirt_sbin_path)
> +
> +if not iscsiadm_prog.found()
> +  if get_option('storage_iscsi').enabled()
> +error('We need iscsiadm for iSCSI storage driver')
> +  else
> +iscsi_enable = false
> +  endif
> +endif

I believe this could be rewritten as

  if not get_option('storage_iscsi').disabled()
iscsi_enable = true
iscsiadm_prog = find_program('iscsiadm', required:
get_option('storage_iscsi'), dirs: libvirt_sbin_path)

if not iscsiadm_prog.found()
  iscsi_enable = false
endif

I see this pattern used elsewhere in the file and it's more concise
while achieving the same result AFAICT.

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH RESEND v3 4/8] meson: move iscsiadm check into storage_iscsi condition

[Pavel Hrdina]

This requires to define the binary name in code because we compile
src/util/viriscsi.c unconditionally.

Signed-off-by: Pavel Hrdina 
---
 meson.build | 22 --
 src/util/viriscsi.h |  2 ++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index ed4afd2052..7eeaa689f9 100644
--- a/meson.build
+++ b/meson.build
@@ -824,7 +824,6 @@ endforeach
 optional_programs = [
   'augparse',
   'flake8',
-  'iscsiadm',
   'pdwtags',
 ]
 
@@ -1742,11 +1741,22 @@ if conf.has('WITH_LIBVIRTD')
 error('Need glusterfs (libgfapi) for gluster storage driver')
   endif
 
-  if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
-use_storage = true
-conf.set('WITH_STORAGE_ISCSI', 1)
-  elif get_option('storage_iscsi').enabled()
-error('We need iscsiadm for iSCSI storage driver')
+  if not get_option('storage_iscsi').disabled()
+iscsi_enable = true
+iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
libvirt_sbin_path)
+
+if not iscsiadm_prog.found()
+  if get_option('storage_iscsi').enabled()
+error('We need iscsiadm for iSCSI storage driver')
+  else
+iscsi_enable = false
+  endif
+endif
+
+if iscsi_enable
+  use_storage = true
+  conf.set('WITH_STORAGE_ISCSI', 1)
+endif
   endif
 
   if not get_option('storage_iscsi_direct').disabled() and libiscsi_dep.found()
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h
index 6d17d139eb..fa7ff280c2 100644
--- a/src/util/viriscsi.h
+++ b/src/util/viriscsi.h
@@ -23,6 +23,8 @@
 
 #include "internal.h"
 
+#define ISCSIADM "iscsiadm"
+
 char *
 virISCSIGetSession(const char *devpath,
bool probe)
-- 
2.31.1