Re: Antw: Re: [RFC 1/3] iscsid: Changes to support the new qedi transport

2016-10-20 Thread Javali, Nilesh 
Hi Chris, Lee,

Adding a field in transport structure, like below,

-- 8< --
diff --git a/usr/initiator_common.c b/usr/initiator_common.c
index dd3f3c4..191e779 100644
--- a/usr/initiator_common.c
+++ b/usr/initiator_common.c
@@ -700,7 +700,7 @@ int iscsi_host_set_net_params(struct iface_rec *iface,
netdev = hinfo.iface.netdev;
}

-   if (strcmp(iface->transport_name, "qedi") && 
net_ifup_netdev(netdev))
+  if (!t->template->no_netdev && net_ifup_netdev(netdev))
log_warning("Could not brining up netdev %s. 
Try running "
"'ifup %s' first if login 
fails.", netdev, netdev);

diff --git a/usr/transport.c b/usr/transport.c
index b933c36..533ba30 100644
--- a/usr/transport.c
+++ b/usr/transport.c
@@ -119,6 +119,7 @@ struct iscsi_transport_template qedi = {
.set_host_ip = SET_HOST_IP_REQ,
.use_boot_info= 1,
.bind_ep_required = 1,
+  .no_netdev = 1,
.ep_connect = ktransport_ep_connect,
.ep_poll = ktransport_ep_poll,
.ep_disconnect= ktransport_ep_disconnect,
diff --git a/usr/transport.h b/usr/transport.h
index 4d3bdbf..b67776b 100644
--- a/usr/transport.h
+++ b/usr/transport.h
@@ -39,6 +39,7 @@ struct iscsi_transport_template {
uint8_t set_host_ip;
uint8_t use_boot_info;
 uint8_t bind_ep_required;
+  uint8_t no_netdev;
int (*ep_connect) (struct iscsi_conn *conn, int non_blocking);
int (*ep_poll) (struct iscsi_conn *conn, int timeout_ms);
void (*ep_disconnect) (struct iscsi_conn *conn);

-- 8< --

I would submit this in next revision.

Thanks,
Nilesh


On 20/10/16, 9:11 PM, "The Lee-Man" 
> wrote:

In this case, I'd like to see you create a "class" of transports that have this 
feature. Or perhaps it can be a field in the transport structure? I dislike 
checking against one transport, as Chris also mentioned.

On Wednesday, October 19, 2016 at 11:38:08 PM UTC-7, Javali, Nilesh wrote:


On 20/10/16, 12:00 PM, 
"open-iscsi@googlegroups.com on behalf of 
Ulrich Windl"  
on behalf of 
ulrich.wi...@rz.uni-regensburg.de> 
wrote:

Chris Leech > schrieb am 19.10.2016 
um 19:14 in Nachricht
<20161019171452.iflp5nibq7yzi...@straylight.hirudinean.org>:
On Wed, Oct 19, 2016 at 01:02:18AM -0400, 
nilesh.jav...@cavium.com wrote:
From: Nilesh Javali >
qedi is not attached to netdev hence avoid suppressing warnings.
Signed-off-by: Manish Rangankar 
>
Signed-off-by: Adheer Chandravanshi 
>
Signed-off-by: Nilesh Javali 
>
---
  usr/initiator_common.c |  2 +-
  usr/transport.c| 12 
  2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/usr/initiator_common.c b/usr/initiator_common.c
index 1d1d822..dd3f3c4 100644
--- a/usr/initiator_common.c
+++ b/usr/initiator_common.c
@@ -700,7 +700,7 @@ int iscsi_host_set_net_params(struct iface_rec *iface,
  netdev = hinfo.iface.netdev;
}

- if (net_ifup_netdev(netdev))
+ if (strcmp(iface->transport_name, "qedi") && net_ifup_netdev(netdev))
  log_warning("Could not brining up netdev %s. Try running "
"'ifup %s' first if login fails.", netdev, netdev);
We're not scattering transport name checks all over the code.

>>> At least the magic string should be replaced by a symbolic constant.


Yes. We would validate netdev using new field in transport template to have 
cleaner solution to suppress
the warnings.


Especially if this is just suppressing the warning level message, while
net_ifup_netdev is probably logging an error?  This needs to be handled
better.
Is this really the first transport we have that wants net params set
from iscsid without exposing a netdev?  This is going to be fun shaking
out all the details now that there's a second user of iscsiuio.

diff --git a/usr/transport.c b/usr/transport.c
index 18b7704..b933c36 100644
--- a/usr/transport.c
+++ b/usr/transport.c
@@ -114,6 +114,17 @@ struct iscsi_transport_template ocs = {
.ep_disconnect= ktransport_ep_disconnect,
  };

+struct iscsi_transport_template qedi = {
+ .name   = "qedi",
+ .set_host_ip  = SET_HOST_IP_REQ,
+ .use_boot_info= 1,
+ .bind_ep_required = 1,
+

Re: Antw: Re: [RFC 1/3] iscsid: Changes to support the new qedi transport

2016-10-20 Thread The Lee-Man 
In this case, I'd like to see you create a "class" of transports that have 
this feature. Or perhaps it can be a field in the transport structure? I 
dislike checking against one transport, as Chris also mentioned.

On Wednesday, October 19, 2016 at 11:38:08 PM UTC-7, Javali, Nilesh wrote:
>
>  
>
>  
>
> On 20/10/16, 12:00 PM, "open-iscsi@googlegroups.com on behalf of Ulrich 
> Windl"  ulrich.wi...@rz.uni-regensburg.de> wrote:
>
>  
>
> Chris Leech  schrieb am 19.10.2016 um 19:14 in 
> Nachricht
>
> <20161019171452.iflp5nibq7yzi...@straylight.hirudinean.org>:
>
> On Wed, Oct 19, 2016 at 01:02:18AM -0400, nilesh.jav...@cavium.com wrote:
>
> From: Nilesh Javali 
>
> qedi is not attached to netdev hence avoid suppressing warnings.
>
> Signed-off-by: Manish Rangankar 
>
> Signed-off-by: Adheer Chandravanshi 
>
> Signed-off-by: Nilesh Javali 
>
> ---
>
>   usr/initiator_common.c |  2 +-
>
>   usr/transport.c| 12 
>
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/usr/initiator_common.c b/usr/initiator_common.c
>
> index 1d1d822..dd3f3c4 100644
>
> --- a/usr/initiator_common.c
>
> +++ b/usr/initiator_common.c
>
> @@ -700,7 +700,7 @@ int iscsi_host_set_net_params(struct iface_rec *iface,
>
>   netdev = hinfo.iface.netdev;
>
> }
>
>   
>
> - if (net_ifup_netdev(netdev))
>
> + if (strcmp(iface->transport_name, "qedi") && net_ifup_netdev(netdev))
>
>   log_warning("Could not brining up netdev %s. Try running "
>
> "'ifup %s' first if login fails.", netdev, 
> netdev);
>
> We're not scattering transport name checks all over the code.
>
>  
>
> >>> At least the magic string should be replaced by a symbolic constant.
>
>  
>
>  
>
> Yes. We would validate netdev using new field in transport template to 
> have cleaner solution to suppress
>
> the warnings.
>
>  
>
>  
>
> Especially if this is just suppressing the warning level message, while
>
> net_ifup_netdev is probably logging an error?  This needs to be handled
>
> better.
>
> Is this really the first transport we have that wants net params set
>
> from iscsid without exposing a netdev?  This is going to be fun shaking
>
> out all the details now that there's a second user of iscsiuio.
>
>
>
> diff --git a/usr/transport.c b/usr/transport.c
>
> index 18b7704..b933c36 100644
>
> --- a/usr/transport.c
>
> +++ b/usr/transport.c
>
> @@ -114,6 +114,17 @@ struct iscsi_transport_template ocs = {
>
> .ep_disconnect= ktransport_ep_disconnect,
>
>   };
>
>   
>
> +struct iscsi_transport_template qedi = {
>
> + .name   = "qedi",
>
> + .set_host_ip  = SET_HOST_IP_REQ,
>
> + .use_boot_info= 1,
>
> + .bind_ep_required = 1,
>
> + .ep_connect = ktransport_ep_connect,
>
> + .ep_poll= ktransport_ep_poll,
>
> + .ep_disconnect= ktransport_ep_disconnect,
>
> + .set_net_config = uip_broadcast_params,
>
> +};
>
> +
>
>   static struct iscsi_transport_template *iscsi_transport_templates[] = {
>
> _tcp,
>
> _iser,
>
> @@ -123,6 +134,7 @@ static struct iscsi_transport_template 
>
> *iscsi_transport_templates[] = {
>
> ,
>
> ,
>
> ,
>
> + ,
>
> NULL
>
>   };
>
>   
>
> -- 
>
> 1.8.3.1
>
> -- 
>
> You received this message because you are subscribed to the Google Groups 
>
> "open-iscsi" group.
>
> To unsubscribe from this group and stop receiving emails from it, send an 
>
> email to open-iscsi+unsubscr...@googlegroups.com.
>
> To post to this group, send email to open-iscsi@googlegroups.com.
>
> Visit this group at https://groups.google.com/group/open-iscsi.
>
> For more options, visit https://groups.google.com/d/optout.
>
>  
>
>  
>
>  
>
>  
>
> -- 
>
> You received this message because you are subscribed to the Google Groups 
> "open-iscsi" group.
>
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to open-iscsi+unsubscr...@googlegroups.com.
>
> To post to this group, send email to open-iscsi@googlegroups.com.
>
> Visit this group at https://groups.google.com/group/open-iscsi.
>
> For more options, visit https://groups.google.com/d/optout.
>
>  
>

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: Antw: Re: [RFC 1/3] iscsid: Changes to support the new qedi transport

2016-10-20 Thread Javali, Nilesh 


On 20/10/16, 12:00 PM, 
"open-iscsi@googlegroups.com on behalf of 
Ulrich Windl"  
on behalf of 
ulrich.wi...@rz.uni-regensburg.de> 
wrote:

Chris Leech > schrieb am 19.10.2016 
um 19:14 in Nachricht
<20161019171452.iflp5nibq7yzi...@straylight.hirudinean.org>:
On Wed, Oct 19, 2016 at 01:02:18AM -0400, 
nilesh.jav...@cavium.com wrote:
From: Nilesh Javali >
qedi is not attached to netdev hence avoid suppressing warnings.
Signed-off-by: Manish Rangankar 
>
Signed-off-by: Adheer Chandravanshi 
>
Signed-off-by: Nilesh Javali 
>
---
  usr/initiator_common.c |  2 +-
  usr/transport.c| 12 
  2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/usr/initiator_common.c b/usr/initiator_common.c
index 1d1d822..dd3f3c4 100644
--- a/usr/initiator_common.c
+++ b/usr/initiator_common.c
@@ -700,7 +700,7 @@ int iscsi_host_set_net_params(struct iface_rec *iface,
  netdev = hinfo.iface.netdev;
}

- if (net_ifup_netdev(netdev))
+ if (strcmp(iface->transport_name, "qedi") && net_ifup_netdev(netdev))
  log_warning("Could not brining up netdev %s. Try running "
"'ifup %s' first if login fails.", netdev, netdev);
We're not scattering transport name checks all over the code.

>>> At least the magic string should be replaced by a symbolic constant.


Yes. We would validate netdev using new field in transport template to have 
cleaner solution to suppress
the warnings.


Especially if this is just suppressing the warning level message, while
net_ifup_netdev is probably logging an error?  This needs to be handled
better.
Is this really the first transport we have that wants net params set
from iscsid without exposing a netdev?  This is going to be fun shaking
out all the details now that there's a second user of iscsiuio.

diff --git a/usr/transport.c b/usr/transport.c
index 18b7704..b933c36 100644
--- a/usr/transport.c
+++ b/usr/transport.c
@@ -114,6 +114,17 @@ struct iscsi_transport_template ocs = {
.ep_disconnect= ktransport_ep_disconnect,
  };

+struct iscsi_transport_template qedi = {
+ .name   = "qedi",
+ .set_host_ip  = SET_HOST_IP_REQ,
+ .use_boot_info= 1,
+ .bind_ep_required = 1,
+ .ep_connect = ktransport_ep_connect,
+ .ep_poll= ktransport_ep_poll,
+ .ep_disconnect= ktransport_ep_disconnect,
+ .set_net_config = uip_broadcast_params,
+};
+
  static struct iscsi_transport_template *iscsi_transport_templates[] = {
_tcp,
_iser,
@@ -123,6 +134,7 @@ static struct iscsi_transport_template
*iscsi_transport_templates[] = {
,
,
,
+ ,
NULL
  };

--
1.8.3.1
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to 
open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to 
open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to 
open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to 
open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Antw: Re: [RFC 1/3] iscsid: Changes to support the new qedi transport

2016-10-20 Thread Ulrich Windl 
>>> Chris Leech  schrieb am 19.10.2016 um 19:14 in Nachricht
<20161019171452.iflp5nibq7yzi...@straylight.hirudinean.org>:
> On Wed, Oct 19, 2016 at 01:02:18AM -0400, nilesh.jav...@cavium.com wrote:
>> From: Nilesh Javali 
>> 
>> qedi is not attached to netdev hence avoid suppressing warnings.
>> 
>> Signed-off-by: Manish Rangankar 
>> Signed-off-by: Adheer Chandravanshi 
>> Signed-off-by: Nilesh Javali 
>> ---
>>  usr/initiator_common.c |  2 +-
>>  usr/transport.c| 12 
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/usr/initiator_common.c b/usr/initiator_common.c
>> index 1d1d822..dd3f3c4 100644
>> --- a/usr/initiator_common.c
>> +++ b/usr/initiator_common.c
>> @@ -700,7 +700,7 @@ int iscsi_host_set_net_params(struct iface_rec *iface,
>>  netdev = hinfo.iface.netdev;
>>  }
>>  
>> -if (net_ifup_netdev(netdev))
>> +if (strcmp(iface->transport_name, "qedi") && net_ifup_netdev(netdev))
>>  log_warning("Could not brining up netdev %s. Try running "
>>  "'ifup %s' first if login fails.", netdev, netdev);
> 
> We're not scattering transport name checks all over the code.

At least the magic string should be replaced by a symbolic constant.

> Especially if this is just suppressing the warning level message, while
> net_ifup_netdev is probably logging an error?  This needs to be handled
> better.
> 
> Is this really the first transport we have that wants net params set
> from iscsid without exposing a netdev?  This is going to be fun shaking
> out all the details now that there's a second user of iscsiuio.
>   
>> diff --git a/usr/transport.c b/usr/transport.c
>> index 18b7704..b933c36 100644
>> --- a/usr/transport.c
>> +++ b/usr/transport.c
>> @@ -114,6 +114,17 @@ struct iscsi_transport_template ocs = {
>>  .ep_disconnect  = ktransport_ep_disconnect,
>>  };
>>  
>> +struct iscsi_transport_template qedi = {
>> +.name   = "qedi",
>> +.set_host_ip= SET_HOST_IP_REQ,
>> +.use_boot_info  = 1,
>> +.bind_ep_required = 1,
>> +.ep_connect = ktransport_ep_connect,
>> +.ep_poll= ktransport_ep_poll,
>> +.ep_disconnect  = ktransport_ep_disconnect,
>> +.set_net_config = uip_broadcast_params,
>> +};
>> +
>>  static struct iscsi_transport_template *iscsi_transport_templates[] = {
>>  _tcp,
>>  _iser,
>> @@ -123,6 +134,7 @@ static struct iscsi_transport_template 
> *iscsi_transport_templates[] = {
>>  ,
>>  ,
>>  ,
>> +,
>>  NULL
>>  };
>>  
>> -- 
>> 1.8.3.1
>> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to open-iscsi+unsubscr...@googlegroups.com.
> To post to this group, send email to open-iscsi@googlegroups.com.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.




-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.