[libvirt] [PATCH 03/11] Stop iSCSI targets automatically logging back in after logout

[Daniel P. Berrange]

On Fri, Nov 19, 2010 at 11:09:31AM -0700, Eric Blake wrote:
> On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
> > The Linux iSCSI initiator toolchain has the dubious feature that
> > if you ever run the 'sendtargets' command to merely query what
> > targets are available from a server, the results will be recorded
> > in /var/lib/iscsi. Any time the '/etc/init.d/iscsi' script runs
> > in the future, it will then automatically login to all those
> > targets. /etc/init.d/iscsi is automatically run whenever a NIC
> > comes online.
> 
> Is that worth reporting as a bug in the iscsi toolchain?  At any rate,
> we need this patch whether or not that tool changes behavior to
> something more sane.  However, I'm not sure this is ready for ack
> without answers to some questions first:

I've already reported it as a bug and it went nowhere productive
with the iSCSI maintainer claiming this insanity is a desirable
feature.

> > +static int
> > +virStorageBackendISCSIGetTargets(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> > +                                 char **const groups,
> > +                                 void *data)
> > +{
> > +    struct virStorageBackendISCSITargetList *list = data;
> > +    char *target;
> > +
> > +    if (!(target = strdup(groups[1]))) {
> > +        virReportOOMError();
> > +        return -1;
> > +    }
> > +
> > +    if (VIR_REALLOC_N(list->targets, list->ntargets + 1) < 0) {
> 
> Up to you if you want to rebase this to use VIR_RESIZE_N (or
> VIR_EXPAND_N), or to just leave that to me as a subsequent followup
> patch (I'm already searching through the code base for other instances
> to convert; one more won't hurt me).

I'll let you change this, there are many more instances of this
in the storage backends already.

> 
> >  static int
> > -virStorageBackendISCSIScanTargets(const char *portal)
> > +virStorageBackendISCSIScanTargets(const char *portal,
> > +                                  const char *initiatoriqn,
> > +                                  size_t *ntargetsret,
> > +                                  char ***targetsret)
> >  {
> > -    const char *const sendtargets[] = {
> > -        ISCSIADM, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, NULL
> > +    /**
> > +     *
> > +     * The output of sendtargets is very simple, just two columns,
> > +     * portal then target name
> > +     *
> > +     * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo0.bf6d84
> > +     * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo1.bf6d84
> > +     * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo2.bf6d84
> > +     * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo3.bf6d84
> > +     */
> > +    const char *regexes[] = {
> > +        "^\\s*(\\S+)\\s+(\\S+)\\s*$"
> 
> \s and \S are GNU-isms, and regcomp() on other platforms will reject it;
> is this regex only used on Linux, or do we need to be portable to iscsi
> implementations on other platforms?

The storage drivers are already full of this regex syntax so
this isn't making it any less portable really.

> 
> > +    };
> > +    int vars[] = { 2 };
> > +    const char *const cmdsendtarget[] = {
> > +        ISCSIADM, "--mode", "discovery", "--type", "sendtargets",
> > +        "--portal", portal, NULL
> >      };
> > -    if (virRun(sendtargets, NULL) < 0) {
> > +    struct virStorageBackendISCSITargetList list;
> > +    int i;
> > +    int exitstatus;
> > +
> > +    memset(&list, 0, sizeof(list));
> > +
> > +    if (virStorageBackendRunProgRegex(NULL, /* No pool for callback */
> > +                                      cmdsendtarget,
> > +                                      1,
> > +                                      regexes,
> > +                                      vars,
> > +                                      virStorageBackendISCSIGetTargets,
> > +                                      &list,
> > +                                      &exitstatus) < 0) {
> >          virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> > -                              _("Failed to run %s to get target list"),
> > -                              sendtargets[0]);
> > +                              "%s", _("lvs command failed"));
> 
> Should this message be about iscsiadm rather than lvs?

Yep.

Daniel