Message ID | 20190916094448.26072-1-jiri@resnulli.us |
---|---|
State | Accepted |
Delegated to: | David Ahern |
Headers | show |
Series | [iproute2-next,v2] devlink: add reload failed indication | expand |
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"?
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.
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.
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.
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'.
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.
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.
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 */