Re: [PATCH] Divide node parameters into 3 categories
Eli Dorfman wrote: Doron Shoham wrote: Hi Mike, Are you ok with this patch? I think so. There was just some minor things I was going to fix when I merged it . For exmaples the timer values are not picked up right away so they need to be attr deffered. I am not going to merge it for the next release (the one that is in rc now), because some apps (installers and mkinitrd type of tools) are not ready for the -f flag and it is too late to change them. So I will do a branch for the next release or merge this first when I make the new release. I was just waiting to see if Hannes fixes the Makefile patch he had. Hi, Did you meant open-iscsi-2.0-869.1 as the next release? The dot releases are just bug fixes. If not, can you estimate in what release it will be merged? 870. Isn't this patch a bug fix as well? For example, changing transport name while session is active, leaves stale session entry. It fixes your bug, but like I said it adds a new problem. If you can figure out a fix that does not add a regression then I can merge it sooner. Your problem also does not exist in git because of the transport_name changes, but I do not like that code for the reasons Pete and I discussed in the other thread and because that breaks existing iser users. We could avoid the iser breakage by fixing iscsi_discovery again, but 869 broke the node.transport support and I do not think that was right to do to distros that were using iscsiadm. And we are going to have other drivers needing to set the transport_name so we need a proper solution to this that we are going to support like other commands. In the end what we need is something that is going to be usable by tools like the distro installers, so that when a user goes to install to a iscsi disk the proper modules get setup or if we want to switch modules we can. And it has to be simple enough that normal users can just use it. If we want to say use iscsi_discovery then we can do that, but that interface has to be made stable so distros can use it - except for the transport_name mess up I have been adding compat code to iscsiadm. So for the installers if we have the user enter the target info, then it runs iscsi_discovery to figure out what module to use, I think that could work. The only problem might be what to do if you have multiple modules that work. Maybe we could make records for all modules, or ask the user which one they wanted to use. OTOH, if we do not use iscsi_discovery then we could just have the user enter in the module to use with the target info (this is what I was thinking when I did the default iser and bnx iface code). I do not know what is best, but what we are doing today does not seem nice. Can't you merge it to the nearest release? If not, when do you plan to release the 870? It's done when it is done :) I wanted to try and get it out when .26 is out because the modules in the tarball releases do not work with that kernel. Also the sysfs code was broken when sysfs compat was not used and in newer kernels that do not have block device symlinks from the scsi_device. So if you want to help you can: 1. Take the kernel modules from upstream 2.6.26-rc4 and update the kernel modules in the open-iscsi git tree to them, and then update the compat patches to apply over the new modules. 2. The sysfs code is now fixed in git. It needs testing for all the kernel combos we support. Checkout the kernels we support and then check to see if when sysfs compat is on or off, if iscsiadm/iscsid works. 3. Remove the default ifaces code with something better. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Doron Shoham wrote: Divide node parameters into 3 categories: a. Immutable - properties that are not allowed to change at all. b. Immediate - properties that are allowed to change and take effect immediately. c. Deferred - properties that are allowed to be changed only while session is not active (or when using -f flag). The change will take effect only next session. Add -f flag (force) to iscsiadm when changing node parameters that are defined as ATTR_DEFERRED. When using this flag, changing of ATTR_DEFERRED parameter while in active session is allowed. The change itself will take effect only in the next session. A warning message will be displayed. Hi Mike, Are you ok with this patch? Thanks, Doron --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Add -f flag (force) when changing node parameters that are
defined as ATTR_DEFERRED.
When using this flag, changing of ATTR_DEFERRED parameter
while in active session can take place.
The change itself will take effect only in the next session.
A warning message is displayed.
Signed-off-by: Doron Shoham [EMAIL PROTECTED]
---
usr/idbm.c |7 ++-
usr/iscsiadm.c | 50 ++
2 files changed, 36 insertions(+), 21 deletions(-)
diff --git a/usr/idbm.c b/usr/idbm.c
index c77042a..35390b3 100644
--- a/usr/idbm.c
+++ b/usr/idbm.c
@@ -593,10 +593,15 @@ static int idbm_verify_param(recinfo_t *info, char *name,
int is_active)
else if (info[i].can_modify == ATTR_DEFERRED) {
if (is_active == 0)
return 0;
- else {
+ else if (is_active == 1) {
log_error(Cannot modify %s. Please logout
before modifing it, name);
return EINVAL;
}
+ else if (is_active == 2) {
+ log_warning(Modify %s while in active session.
\
+ \n\t Changes will take effect only in the next
login, name);
+ return 0;
+ }
}
else if (info[i].can_modify == ATTR_IMMUTABLE) {
log_error(Cannot modify %s. This value is immutable
diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c
index 847393d..a79c7e9 100644
--- a/usr/iscsiadm.c
+++ b/usr/iscsiadm.c
@@ -85,9 +85,10 @@ static struct option const long_options[] =
{show, no_argument, NULL, 'S'},
{version, no_argument, NULL, 'V'},
{help, no_argument, NULL, 'h'},
+ {force, no_argument, NULL, 'f'},
{NULL, 0, NULL, 0},
};
-static char *short_options = RlVhm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:u;
+static char *short_options = RlVhfm:p:P:T:H:I:U:k:L:d:r:n:v:o:sSt:u;
static void usage(int status)
{
@@ -1757,7 +1758,7 @@ static void catch_sigint( int signo ) {
}
/* TODO: merge iter helpers and clean them up, so we can use them here */
-static int exec_iface_op(idbm_t *db, int op, int do_show, int info_level,
+static int exec_iface_op(idbm_t *db, int op, int do_show, int do_force, int
info_level,
struct iface_rec *iface, char *name, char *value)
{
struct db_set_param set_param;
@@ -1873,11 +1874,14 @@ delete_fail:
set_param.value = value;
/* pass rec's iface because it has the db values */
- if (check_for_session_through_iface(rec))
- set_param.connected = 1;
- else
+ if (check_for_session_through_iface(rec)) {
+ if (do_force == 1)
+ set_param.connected = 2;
+ else
+ set_param.connected = 1;
+ } else
set_param.connected = 0;
-
+
rc = iface_conf_update(db, set_param, rec-iface);
if (rc)
goto update_fail;
@@ -1917,7 +1921,7 @@ update_fail:
/* TODO cleanup arguments */
static int exec_node_op(idbm_t *db, int op, int do_login, int do_logout,
- int do_show, int do_rescan, int do_stats,
+ int do_show, int do_rescan, int do_stats, int do_force,
int info_level, struct node_rec *rec,
char *name, char *value)
{
@@ -2011,12 +2015,15 @@ static int exec_node_op(idbm_t *db, int op, int
do_login, int do_logout,
set_param.db = db;
set_param.name = name;
set_param.value = value;
- if (check_for_session_through_iface(rec))
- set_param.connected = 1;
- else
+ if (check_for_session_through_iface(rec)) {
+ if (do_force == 1)
+ set_param.connected = 2;
+ else
+ set_param.connected = 1;
+ } else
set_param.connected = 0;
- if (for_each_rec(db, rec, set_param, idbm_node_set_param))
+ if (for_each_rec(db, rec, set_param, idbm_node_set_param))
rc = -1;
goto out;
} else if (op == OP_DELETE) {
@@ -2153,7 +2160,7 @@ main(int argc, char **argv)
{
char *ip = NULL, *name = NULL, *value = NULL;
char *targetname = NULL, *group_session_mgmt_mode = NULL;
- int ch, longindex, mode=-1, port=-1, do_login=0, do_rescan=0;
+ int ch, longindex, mode=-1, port=-1, do_login=0, do_rescan=0,
do_force=0;
int rc=0, sid=-1, op=OP_NOOP, type=-1, do_logout=0, do_stats=0;
int
Re: [PATCH] Divide node parameters into 3 categories
Doron Shoham wrote:
Mike Christie wrote:
Doron Shoham wrote:
Mike Christie wrote:
Doron Shoham wrote:
Divide node parameters into 3 categories:
a. Immutable - properties that are not allowed to change at all.
b. Immediate - properties that are allowed to change and take effect
immediately.
c. Deferred - properties that are allowed to change but will take
effect only next
session.
@@ -588,13 +588,22 @@ static int idbm_verify_param(recinfo_t *info,
char *name)
continue;
log_debug(7, verify %s %d\n, name, info[i].can_modify);
-if (info[i].can_modify)
+if (info[i].can_modify == ATTR_IMMEDIATE)
return 0;
-else {
-log_error(Cannot modify %s. It is used to look up
- the record and cannot be changed., name);
-return EINVAL;
+else if (info[i].can_modify == ATTR_DEFERRED) {
+if (is_active == 0)
+return 0;
+else {
+log_error(Cannot modify %s. Please logout before
modifing it, name);
+return EINVAL;
+}
+}
+else if (info[i].can_modify == ATTR_IMMUTABLE) {
+log_error(Cannot modify %s. This value is immutable
+and cannot be modified, name);
+return EINVAL;
}
I was thinking about this and there is no reason that we cannot update
the values that are marked ATTR_DEFERRED when a session is logged in.
The description of C) in the first snippet and what we do in
idbm_verify_param do not match up, because idbm_verify_param is not
going to let you update the value if we are logged in. Maybe we just
want to print a warning letting the user know that they have to relogin
to use the new value.
Yes, you are right.
The description should have been something like:
Deferred - properties that are allowed to be changed only while
session is not active. They will take effect next session.
If we let people to change them while in active session it can be
confusing - after the change, if someone will query the node properties
he will see the new parameters while these parameters are not
the actual parameters which the session is used.
I think this is fine because the node record values are what is stored
in the db. It is not supposed to be what we are using now. session mode
should display the values we negotiated for and what was requested.
Also, if you are doing iscsi boot you will never be able to change a
value for the session used for root.
When using iscsi boot, I think that changing the node parameters
while logged in will not effect next session
No, it will :) For Fedora/RHEL, iscsistart only takes the basic settings
to get booted. When iscsid starts up it will then look in the db for all
the other settings like timers, segmentlengths, queue depths, etc and
resetup the session to use them. For suse, I think they copy the values
from the db to the initrd so either way for those two distros we need to
be able to edit a running sessions db values for the next startup and we
cannot bring down the session because it is root.
(you will have to change the default iscsi boot parameters).
Yeah, unfortunately the boot/root params are picked up from the db.
Another solution is to add -f flag (force).
Using this flag will change the parameter while in active session -
still it will display a warning message that the change will only
effect the next session.
That sounds fine.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Doron Shoham wrote:
Mike Christie wrote:
Doron Shoham wrote:
Divide node parameters into 3 categories:
a. Immutable - properties that are not allowed to change at all.
b. Immediate - properties that are allowed to change and take effect
immediately.
c. Deferred - properties that are allowed to change but will take
effect only next
session.
@@ -588,13 +588,22 @@ static int idbm_verify_param(recinfo_t *info,
char *name)
continue;
log_debug(7, verify %s %d\n, name, info[i].can_modify);
-if (info[i].can_modify)
+if (info[i].can_modify == ATTR_IMMEDIATE)
return 0;
-else {
-log_error(Cannot modify %s. It is used to look up
- the record and cannot be changed., name);
-return EINVAL;
+else if (info[i].can_modify == ATTR_DEFERRED) {
+if (is_active == 0)
+return 0;
+else {
+log_error(Cannot modify %s. Please logout before
modifing it, name);
+return EINVAL;
+}
+}
+else if (info[i].can_modify == ATTR_IMMUTABLE) {
+log_error(Cannot modify %s. This value is immutable
+and cannot be modified, name);
+return EINVAL;
}
I was thinking about this and there is no reason that we cannot update
the values that are marked ATTR_DEFERRED when a session is logged in.
The description of C) in the first snippet and what we do in
idbm_verify_param do not match up, because idbm_verify_param is not
going to let you update the value if we are logged in. Maybe we just
want to print a warning letting the user know that they have to relogin
to use the new value.
Yes, you are right.
The description should have been something like:
Deferred - properties that are allowed to be changed only while
session is not active. They will take effect next session.
If we let people to change them while in active session it can be
confusing - after the change, if someone will query the node properties
he will see the new parameters while these parameters are not
the actual parameters which the session is used.
I think this is fine because the node record values are what is stored
in the db. It is not supposed to be what we are using now. session mode
should display the values we negotiated for and what was requested.
Also, if you are doing iscsi boot you will never be able to change a
value for the session used for root.
And another problem is, again, if someone changes the
transport type while logged in, bad things happen.
We can make it so the transport type is not changable while logged in.
We do that today already in the git tree.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Mike Christie wrote: We can make it so the transport type is not changable while logged in. We do that today already in the git tree. Oh yeah, we were probably both right :) iface.transport_name is checked, but node.transport_name is not, so like we said before we need to update iscsi_discovery and remove the broken compat code or I could have added a check for node.transport_name. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Mike Christie wrote:
Doron Shoham wrote:
Divide node parameters into 3 categories:
a. Immutable - properties that are not allowed to change at all.
b. Immediate - properties that are allowed to change and take effect
immediately.
c. Deferred - properties that are allowed to change but will take
effect only next
session.
@@ -588,13 +588,22 @@ static int idbm_verify_param(recinfo_t *info,
char *name)
continue;
log_debug(7, verify %s %d\n, name, info[i].can_modify);
-if (info[i].can_modify)
+if (info[i].can_modify == ATTR_IMMEDIATE)
return 0;
-else {
-log_error(Cannot modify %s. It is used to look up
- the record and cannot be changed., name);
-return EINVAL;
+else if (info[i].can_modify == ATTR_DEFERRED) {
+if (is_active == 0)
+return 0;
+else {
+log_error(Cannot modify %s. Please logout before
modifing it, name);
+return EINVAL;
+}
+}
+else if (info[i].can_modify == ATTR_IMMUTABLE) {
+log_error(Cannot modify %s. This value is immutable
+and cannot be modified, name);
+return EINVAL;
}
I was thinking about this and there is no reason that we cannot update
the values that are marked ATTR_DEFERRED when a session is logged in.
The description of C) in the first snippet and what we do in
idbm_verify_param do not match up, because idbm_verify_param is not
going to let you update the value if we are logged in. Maybe we just
want to print a warning letting the user know that they have to relogin
to use the new value.
Yes, you are right.
The description should have been something like:
Deferred - properties that are allowed to be changed only while
session is not active. They will take effect next session.
If we let people to change them while in active session it can be
confusing - after the change, if someone will query the node properties
he will see the new parameters while these parameters are not
the actual parameters which the session is used.
And another problem is, again, if someone changes the
transport type while logged in, bad things happen.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Doron Shoham wrote:
Divide node parameters into 3 categories:
a. Immutable - properties that are not allowed to change at all.
b. Immediate - properties that are allowed to change and take effect
immediately.
c. Deferred - properties that are allowed to change but will take effect
only next
session.
Thanks for doing this. It looks pretty good.
+
+typedef enum modify_attr_e {
+ATTR_IMMUTABLE, /*can not be changed at all*/
+ATTR_DEFERRED, /*can be changed only when session is not active*/
+ATTR_IMMEDIATE /*can be changed while in session*/
+} modify_attr_e;
When I merge this, I am just going to add a prefix on the ATTR_* values.
Does something like maybe IDBM_ATTR_* sound ok?
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH] Divide node parameters into 3 categories
Doron Shoham wrote:
Divide node parameters into 3 categories:
a. Immutable - properties that are not allowed to change at all.
b. Immediate - properties that are allowed to change and take effect
immediately.
c. Deferred - properties that are allowed to change but will take effect
only next
session.
@@ -588,13 +588,22 @@ static int idbm_verify_param(recinfo_t *info, char
*name)
continue;
log_debug(7, verify %s %d\n, name, info[i].can_modify);
- if (info[i].can_modify)
+ if (info[i].can_modify == ATTR_IMMEDIATE)
return 0;
- else {
- log_error(Cannot modify %s. It is used to look up
- the record and cannot be changed., name);
- return EINVAL;
+ else if (info[i].can_modify == ATTR_DEFERRED) {
+ if (is_active == 0)
+ return 0;
+ else {
+ log_error(Cannot modify %s. Please logout
before modifing it, name);
+ return EINVAL;
+ }
+ }
+ else if (info[i].can_modify == ATTR_IMMUTABLE) {
+ log_error(Cannot modify %s. This value is immutable
+ and cannot be modified, name);
+return EINVAL;
}
I was thinking about this and there is no reason that we cannot update
the values that are marked ATTR_DEFERRED when a session is logged in.
The description of C) in the first snippet and what we do in
idbm_verify_param do not match up, because idbm_verify_param is not
going to let you update the value if we are logged in. Maybe we just
want to print a warning letting the user know that they have to relogin
to use the new value.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
