Re: [libvirt] [PATCH 03/11] Stop iSCSI targets automatically logging back in after logout
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