Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1673393 > unrolled thread

Re: [PATCH net-next v3 1/6] vxlan: refactor verification and application of configuration

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next v3 1/6] vxlan: refactor verification and  application of configuration Jiri Benc <jbenc@redhat.com> - 2017-06-23 11:00 +0200
    Re: [PATCH net-next v3 1/6] vxlan: refactor verification and  application of configuration Matthias Schiffer <mschiffer@universe-factory.net> - 2017-06-23 12:20 +0200
      Re: [PATCH net-next v3 1/6] vxlan: refactor verification and  application of configuration Johannes Berg <johannes@sipsolutions.net> - 2017-06-23 12:30 +0200
        Re: [PATCH net-next v3 1/6] vxlan: refactor verification and  application of configuration Matthias Schiffer <mschiffer@universe-factory.net> - 2017-06-23 14:10 +0200
          Re: [PATCH net-next v3 1/6] vxlan: refactor verification and  application of configuration Johannes Berg <johannes@sipsolutions.net> - 2017-06-23 15:40 +0200

#1673393 — Re: [PATCH net-next v3 1/6] vxlan: refactor verification and application of configuration

This patchset looks good overall (would send my Acked-by for most of
this but I'm late).

On Mon, 19 Jun 2017 10:03:55 +0200, Matthias Schiffer wrote:
> Log messages in these
> functions are removed, as it is generally unexpected to find error output
> for netlink requests in the kernel log. Userspace should be able to handle
> errors based on the error codes returned via netlink just fine.

However, this is not really true. It's impossible to find out what went
wrong when you use e.g. iproute2 to configure a vxlan link.

We really need to convert the kernel log messages to the extended
netlink errors. Since you removed them prematurely, could you please
work on that?

Thanks,

 Jiri

[toc] | [next] | [standalone]

#1673455

[Multipart message — attachments visible in raw view] — view raw

On 06/23/2017 10:52 AM, Jiri Benc wrote:
> This patchset looks good overall (would send my Acked-by for most of
> this but I'm late).
> 
> On Mon, 19 Jun 2017 10:03:55 +0200, Matthias Schiffer wrote:
>> Log messages in these
>> functions are removed, as it is generally unexpected to find error output
>> for netlink requests in the kernel log. Userspace should be able to handle
>> errors based on the error codes returned via netlink just fine.
> 
> However, this is not really true. It's impossible to find out what went
> wrong when you use e.g. iproute2 to configure a vxlan link.
> 
> We really need to convert the kernel log messages to the extended
> netlink errors. Since you removed them prematurely, could you please
> work on that?
> 
> Thanks,
> 
>  Jiri
> 

I was told the extended netlink error facilities were not ready yet, has
that changed since the last release?

Off the top of my head, I can't think of any other setting I can do with
iproute2 that will write its errors in the kernel log; but there are quite
a lot settings that will just return a very unspecific error code. Isn't it
more common for the userspace tool to handle diagnostics in such cases?

Anyways, I will gladly work on improving the error handling if someone can
give me a pointer how these extended netlink errors are used.

Matthias

[toc] | [prev] | [next] | [standalone]

#1673462

On Fri, 2017-06-23 at 12:13 +0200, Matthias Schiffer wrote:
> 
> I was told the extended netlink error facilities were not ready yet,
> has that changed since the last release?

Yes, the facility is in the kernel tree now.

> Anyways, I will gladly work on improving the error handling if
> someone can give me a pointer how these extended netlink errors are
> used.

Just grep for 'netlink_ext_ack' :)

johannes

[toc] | [prev] | [next] | [standalone]

#1673504

[Multipart message — attachments visible in raw view] — view raw

On 06/23/2017 12:23 PM, Johannes Berg wrote:
> On Fri, 2017-06-23 at 12:13 +0200, Matthias Schiffer wrote:
>>
>> I was told the extended netlink error facilities were not ready yet,
>> has that changed since the last release?
> 
> Yes, the facility is in the kernel tree now.
> 
>> Anyways, I will gladly work on improving the error handling if
>> someone can give me a pointer how these extended netlink errors are
>> used.
> 
> Just grep for 'netlink_ext_ack' :)
> 
> johannes
> 

Thanks for the hint.

It seems though that rtnl_link_ops.newlink/changelink don't allow passing
the extack yet... how do we proceed here? Treewide change (maybe by someone
who knows their Coccinelle-fu?), or would the introduction of new versions
of the newlink and changelink fields be more acceptable, so drivers can be
moved to the new API one by one?

Matthias

[toc] | [prev] | [next] | [standalone]

#1673558

On Fri, 2017-06-23 at 14:02 +0200, Matthias Schiffer wrote:
> 
> It seems though that rtnl_link_ops.newlink/changelink don't allow
> passing the extack yet... how do we proceed here? Treewide change
> (maybe by someone who knows their Coccinelle-fu?), or would the
> introduction of new versions of the newlink and changelink fields be
> more acceptable, so drivers can be moved to the new API one by one?

I think treewide change is easy enough, this seems to work:

@ops1@
identifier newfn, ops;
@@
static struct rtnl_link_ops ops = {
	.newlink = newfn,
...
};

@@
identifier ops1.newfn;
identifier src_net, dev, tb, data;
@@
-int newfn(struct net *src_net, struct net_device *dev,
-	   struct nlattr *tb[], struct nlattr *data[])
+int newfn(struct net *src_net, struct net_device *dev,
+	   struct nlattr *tb[], struct nlattr *data[],
+	   struct netlink_ext_ack *extack)
{...}

@ops2@
identifier chfn, ops;
@@
static struct rtnl_link_ops ops = {
	.changelink = chfn,
...
};

@@
identifier ops2.chfn;
identifier dev, tb, data;
@@
-int chfn(struct net_device *dev,
-	  struct nlattr *tb[], struct nlattr *data[])
+int chfn(struct net_device *dev,
+	  struct nlattr *tb[], struct nlattr *data[],
+	  struct netlink_ext_ack *extack)
{...}

I guess if there are any stragglers you'd find them by compile-testing
:)

johannes

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web