* [patch iproute2-next v2] devlink: add reload failed indication
@ 2019-09-16 9:44 Jiri Pirko
2019-09-17 16:46 ` David Ahern
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2019-09-16 9:44 UTC (permalink / raw)
To: netdev; +Cc: dsahern, stephen, idosch, jakub.kicinski, tariqt, mlxsw
From: Jiri Pirko <jiri@mellanox.com>
Add indication about previous failed devlink reload.
Example outputs:
$ devlink dev
netdevsim/netdevsim10: reload_failed true
$ devlink dev -j -p
{
"dev": {
"netdevsim/netdevsim10": {
"reload_failed": true
}
}
}
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- added patch description including example
---
devlink/devlink.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 15877a04f5d6..da62c144d5d5 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -450,6 +450,7 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
[DEVLINK_ATTR_TRAP_GENERIC] = MNL_TYPE_FLAG,
[DEVLINK_ATTR_TRAP_METADATA] = MNL_TYPE_NESTED,
[DEVLINK_ATTR_TRAP_GROUP_NAME] = MNL_TYPE_STRING,
+ [DEVLINK_ATTR_RELOAD_FAILED] = MNL_TYPE_U8,
};
static const enum mnl_attr_data_type
@@ -1949,11 +1950,6 @@ static void pr_out_region_chunk(struct dl *dl, uint8_t *data, uint32_t len,
pr_out_region_chunk_end(dl);
}
-static void pr_out_dev(struct dl *dl, struct nlattr **tb)
-{
- pr_out_handle(dl, tb);
-}
-
static void pr_out_section_start(struct dl *dl, const char *name)
{
if (dl->json_output) {
@@ -2649,11 +2645,23 @@ static int cmd_dev_show_cb(const struct nlmsghdr *nlh, void *data)
struct dl *dl = data;
struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+ uint8_t reload_failed = 0;
mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME])
return MNL_CB_ERROR;
- pr_out_dev(dl, tb);
+
+ if (tb[DEVLINK_ATTR_RELOAD_FAILED])
+ reload_failed = mnl_attr_get_u8(tb[DEVLINK_ATTR_RELOAD_FAILED]);
+
+ if (reload_failed) {
+ __pr_out_handle_start(dl, tb, true, false);
+ pr_out_bool(dl, "reload_failed", true);
+ pr_out_handle_end(dl);
+ } else {
+ pr_out_handle(dl, tb);
+ }
+
return MNL_CB_OK;
}
@@ -3991,7 +3999,7 @@ static int cmd_mon_show_cb(const struct nlmsghdr *nlh, void *data)
if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME])
return MNL_CB_ERROR;
pr_out_mon_header(genl->cmd);
- pr_out_dev(dl, tb);
+ pr_out_handle(dl, tb);
break;
case DEVLINK_CMD_PORT_GET: /* fall through */
case DEVLINK_CMD_PORT_SET: /* fall through */
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-16 9:44 [patch iproute2-next v2] devlink: add reload failed indication Jiri Pirko
@ 2019-09-17 16:46 ` David Ahern
2019-09-17 18:36 ` Jiri Pirko
0 siblings, 1 reply; 8+ messages in thread
From: David Ahern @ 2019-09-17 16:46 UTC (permalink / raw)
To: Jiri Pirko, netdev; +Cc: stephen, idosch, jakub.kicinski, tariqt, mlxsw
On 9/16/19 3:44 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Add indication about previous failed devlink reload.
>
> Example outputs:
>
> $ devlink dev
> netdevsim/netdevsim10: reload_failed true
odd output to user. Why not just "reload failed"?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-17 16:46 ` David Ahern
@ 2019-09-17 18:36 ` Jiri Pirko
2019-09-17 23:46 ` David Ahern
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2019-09-17 18:36 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>On 9/16/19 3:44 AM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Add indication about previous failed devlink reload.
>>
>> Example outputs:
>>
>> $ devlink dev
>> netdevsim/netdevsim10: reload_failed true
>
>odd output to user. Why not just "reload failed"?
Well it is common to have "name value". The extra space would seem
confusing for the reader..
Also it is common to have "_" instead of space for the output in cases
like this.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-17 18:36 ` Jiri Pirko
@ 2019-09-17 23:46 ` David Ahern
2019-09-18 7:37 ` Jiri Pirko
0 siblings, 1 reply; 8+ messages in thread
From: David Ahern @ 2019-09-17 23:46 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
On 9/17/19 12:36 PM, Jiri Pirko wrote:
> Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>> On 9/16/19 3:44 AM, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Add indication about previous failed devlink reload.
>>>
>>> Example outputs:
>>>
>>> $ devlink dev
>>> netdevsim/netdevsim10: reload_failed true
>>
>> odd output to user. Why not just "reload failed"?
>
> Well it is common to have "name value". The extra space would seem
> confusing for the reader..
> Also it is common to have "_" instead of space for the output in cases
> like this.
>
I am not understanding your point.
"reload failed" is still a name/value pair. It is short and to the point
as to what it indicates. There is no need for the name in the uapi (ie.,
the name of the netlink attribute) to be dumped here.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-17 23:46 ` David Ahern
@ 2019-09-18 7:37 ` Jiri Pirko
2019-09-18 20:01 ` David Ahern
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2019-09-18 7:37 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
Wed, Sep 18, 2019 at 01:46:13AM CEST, dsahern@gmail.com wrote:
>On 9/17/19 12:36 PM, Jiri Pirko wrote:
>> Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>>> On 9/16/19 3:44 AM, Jiri Pirko wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> Add indication about previous failed devlink reload.
>>>>
>>>> Example outputs:
>>>>
>>>> $ devlink dev
>>>> netdevsim/netdevsim10: reload_failed true
>>>
>>> odd output to user. Why not just "reload failed"?
>>
>> Well it is common to have "name value". The extra space would seem
>> confusing for the reader..
>> Also it is common to have "_" instead of space for the output in cases
>> like this.
>>
>
>I am not understanding your point.
>
>"reload failed" is still a name/value pair. It is short and to the point
>as to what it indicates. There is no need for the name in the uapi (ie.,
>the name of the netlink attribute) to be dumped here.
Ah, got it. Well it is a bool value, that means it is "true" or "false".
In json output, it is True of False. App processing json would have to
handle this case in a special way.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-18 7:37 ` Jiri Pirko
@ 2019-09-18 20:01 ` David Ahern
2019-09-18 20:23 ` Jiri Pirko
0 siblings, 1 reply; 8+ messages in thread
From: David Ahern @ 2019-09-18 20:01 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
On 9/18/19 1:37 AM, Jiri Pirko wrote:
> Wed, Sep 18, 2019 at 01:46:13AM CEST, dsahern@gmail.com wrote:
>> On 9/17/19 12:36 PM, Jiri Pirko wrote:
>>> Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>>>> On 9/16/19 3:44 AM, Jiri Pirko wrote:
>>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>>
>>>>> Add indication about previous failed devlink reload.
>>>>>
>>>>> Example outputs:
>>>>>
>>>>> $ devlink dev
>>>>> netdevsim/netdevsim10: reload_failed true
>>>>
>>>> odd output to user. Why not just "reload failed"?
>>>
>>> Well it is common to have "name value". The extra space would seem
>>> confusing for the reader..
>>> Also it is common to have "_" instead of space for the output in cases
>>> like this.
>>>
>>
>> I am not understanding your point.
>>
>> "reload failed" is still a name/value pair. It is short and to the point
>> as to what it indicates. There is no need for the name in the uapi (ie.,
>> the name of the netlink attribute) to be dumped here.
>
> Ah, got it. Well it is a bool value, that means it is "true" or "false".
> In json output, it is True of False. App processing json would have to
> handle this case in a special way.
>
Technically it is a u8. But really I do not understand why it is
RELOAD_FAILED and not RELOAD_STATUS which is more generic and re-usable.
e.g,. 'none', 'failed', 'success'.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-18 20:01 ` David Ahern
@ 2019-09-18 20:23 ` Jiri Pirko
2019-09-19 14:49 ` David Ahern
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2019-09-18 20:23 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
Wed, Sep 18, 2019 at 10:01:31PM CEST, dsahern@gmail.com wrote:
>On 9/18/19 1:37 AM, Jiri Pirko wrote:
>> Wed, Sep 18, 2019 at 01:46:13AM CEST, dsahern@gmail.com wrote:
>>> On 9/17/19 12:36 PM, Jiri Pirko wrote:
>>>> Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>>>>> On 9/16/19 3:44 AM, Jiri Pirko wrote:
>>>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>>>
>>>>>> Add indication about previous failed devlink reload.
>>>>>>
>>>>>> Example outputs:
>>>>>>
>>>>>> $ devlink dev
>>>>>> netdevsim/netdevsim10: reload_failed true
>>>>>
>>>>> odd output to user. Why not just "reload failed"?
>>>>
>>>> Well it is common to have "name value". The extra space would seem
>>>> confusing for the reader..
>>>> Also it is common to have "_" instead of space for the output in cases
>>>> like this.
>>>>
>>>
>>> I am not understanding your point.
>>>
>>> "reload failed" is still a name/value pair. It is short and to the point
>>> as to what it indicates. There is no need for the name in the uapi (ie.,
>>> the name of the netlink attribute) to be dumped here.
>>
>> Ah, got it. Well it is a bool value, that means it is "true" or "false".
>> In json output, it is True of False. App processing json would have to
>> handle this case in a special way.
>>
>
>Technically it is a u8. But really I do not understand why it is
>RELOAD_FAILED and not RELOAD_STATUS which is more generic and re-usable.
>e.g,. 'none', 'failed', 'success'.
I was thinking about that. But I was not able to figure out any other
possible values. So it is bool. For indication of some other status,
there would have to be independent bool/othertype anyway.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch iproute2-next v2] devlink: add reload failed indication
2019-09-18 20:23 ` Jiri Pirko
@ 2019-09-19 14:49 ` David Ahern
0 siblings, 0 replies; 8+ messages in thread
From: David Ahern @ 2019-09-19 14:49 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, stephen, idosch, jakub.kicinski, tariqt, mlxsw
On 9/18/19 2:23 PM, Jiri Pirko wrote:
> Wed, Sep 18, 2019 at 10:01:31PM CEST, dsahern@gmail.com wrote:
>> On 9/18/19 1:37 AM, Jiri Pirko wrote:
>>> Wed, Sep 18, 2019 at 01:46:13AM CEST, dsahern@gmail.com wrote:
>>>> On 9/17/19 12:36 PM, Jiri Pirko wrote:
>>>>> Tue, Sep 17, 2019 at 06:46:31PM CEST, dsahern@gmail.com wrote:
>>>>>> On 9/16/19 3:44 AM, Jiri Pirko wrote:
>>>>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>>>>
>>>>>>> Add indication about previous failed devlink reload.
>>>>>>>
>>>>>>> Example outputs:
>>>>>>>
>>>>>>> $ devlink dev
>>>>>>> netdevsim/netdevsim10: reload_failed true
>>>>>>
>>>>>> odd output to user. Why not just "reload failed"?
>>>>>
>>>>> Well it is common to have "name value". The extra space would seem
>>>>> confusing for the reader..
>>>>> Also it is common to have "_" instead of space for the output in cases
>>>>> like this.
>>>>>
>>>>
>>>> I am not understanding your point.
>>>>
>>>> "reload failed" is still a name/value pair. It is short and to the point
>>>> as to what it indicates. There is no need for the name in the uapi (ie.,
>>>> the name of the netlink attribute) to be dumped here.
>>>
>>> Ah, got it. Well it is a bool value, that means it is "true" or "false".
>>> In json output, it is True of False. App processing json would have to
>>> handle this case in a special way.
>>>
>>
>> Technically it is a u8. But really I do not understand why it is
>> RELOAD_FAILED and not RELOAD_STATUS which is more generic and re-usable.
>> e.g,. 'none', 'failed', 'success'.
>
> I was thinking about that. But I was not able to figure out any other
> possible values. So it is bool. For indication of some other status,
> there would have to be independent bool/othertype anyway.
>
applied to iproute2-next, but I think this API is reactionary and not
very good from a user perspective.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-19 14:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-16 9:44 [patch iproute2-next v2] devlink: add reload failed indication Jiri Pirko
2019-09-17 16:46 ` David Ahern
2019-09-17 18:36 ` Jiri Pirko
2019-09-17 23:46 ` David Ahern
2019-09-18 7:37 ` Jiri Pirko
2019-09-18 20:01 ` David Ahern
2019-09-18 20:23 ` Jiri Pirko
2019-09-19 14:49 ` David Ahern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).