Hi Tom,
> -----Original Message-----
> From: Tom Gundersen [mailto:t...@jklm.no]
> Sent: den 15 januari 2014 14:23
> To: Umut Tezduyar Lindskog
> Cc: systemd Mailing List; Umut Tezduyar Lindskog
> Subject: Re: [systemd-devel] [PATCH] udev: ifname matches given mac
>
> Hi Umut,
>
> I'm not really convinced about this. I think we should be fairly conservative
> about what names we consider persistent.
>
> The way NamePolicy=mac currently works is that it will base the name on the
> hardware mac address (i.e., not randomly generated by the kernel, set by
> userspace etc), and only do that if we deem it to be sane.
My thought was as a user, NamePolicy=mac meant that whatever the end mac
address is going to be will form the basis for my interface name. With this
thought, I realized my interface name was not same as my mac address since I
set the mac address from userspace. I thought this is a bug but I also see your
point of view which is NamePolicy=mac means hardware's mac address as also
stated in the man page.
Thanks
>
> With your patch you also allow using the mac address set by us (but note that
> if it is set by other userspace it is still not used). In the case of
> MACPolicy=random, this is certainly the wrong thing to do, and even in the
> case of MACAddress=persistent or MACAddress=XX:XX:XX:XX:XX:XX, I'm not
> entirely convinced as these values are not really "permanent", in the sense
> that they are part of the configuration and not of the hardware.
>
> Any other opinions on this?
>
> Cheers,
>
> Tom
>
> On Wed, Jan 15, 2014 at 1:39 PM, Umut Tezduyar Lindskog
> <umut.tezdu...@axis.com> wrote:
> > This covers the case where NamePolicy has mac in it and MACAddress is
> > given.
> >
> > Ex:
> >
> > NamePolicy=mac
> > MACAddress=00:00:00:00:00:00
> >
> > Interface name becomes [en|wl]x000000000000
> >
> > Also cleanup rtnl_set_link_properties
> > ---
> > src/libsystemd/rtnl-util.c | 16 ++-------
> > src/udev/net/link-config.c | 56
> > +++++++++++++++++++------------
> > src/udev/net/link-config.h | 2 +-
> > src/udev/udev-builtin-net_setup_link.c | 8 +++-
> > 4 files changed, 44 insertions(+), 38 deletions(-)
> >
> > diff --git a/src/libsystemd/rtnl-util.c b/src/libsystemd/rtnl-util.c
> > index dfc0050..cebda2e 100644
> > --- a/src/libsystemd/rtnl-util.c
> > +++ b/src/libsystemd/rtnl-util.c
> > @@ -52,7 +52,6 @@ int rtnl_set_link_name(sd_rtnl *rtnl, int ifindex,
> > const char *name) { int rtnl_set_link_properties(sd_rtnl *rtnl, int
> > ifindex,
> const char *alias,
> > const struct ether_addr *mac, unsigned mtu) {
> > _cleanup_sd_rtnl_message_unref_ sd_rtnl_message *message =
> NULL;
> > - bool need_update = false;
> > int r;
> >
> > assert(rtnl);
> > @@ -69,32 +68,23 @@ int rtnl_set_link_properties(sd_rtnl *rtnl, int ifindex,
> const char *alias,
> > r = sd_rtnl_message_append_string(message, IFLA_IFALIAS,
> > alias);
> > if (r < 0)
> > return r;
> > -
> > - need_update = true;
> > -
> > }
> >
> > if (mac) {
> > r = sd_rtnl_message_append_ether_addr(message,
> IFLA_ADDRESS, mac);
> > if (r < 0)
> > return r;
> > -
> > - need_update = true;
> > }
> >
> > if (mtu > 0) {
> > r = sd_rtnl_message_append_u32(message, IFLA_MTU, mtu);
> > if (r < 0)
> > return r;
> > -
> > - need_update = true;
> > }
> >
> > - if (need_update) {
> > - r = sd_rtnl_call(rtnl, message, 0, NULL);
> > - if (r < 0)
> > - return r;
> > - }
> > + r = sd_rtnl_call(rtnl, message, 0, NULL);
> > + if (r < 0)
> > + return r;
> >
> > return 0;
> > }
> > diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c
> > index bd97cd8..e0a58ff 100644
> > --- a/src/udev/net/link-config.c
> > +++ b/src/udev/net/link-config.c
> > @@ -342,7 +342,7 @@ static int get_mac(struct udev_device *device, bool
> want_random, struct ether_ad
> > return 0;
> > }
> >
> > -int link_config_apply(link_config_ctx *ctx, link_config *config,
> > struct udev_device *device, const char **name) {
> > +int link_config_apply(link_config_ctx *ctx, link_config *config,
> > +struct udev_device *device, const char **name, bool *free_name) {
> > const char *old_name;
> > const char *new_name = NULL;
> > struct ether_addr generated_mac; @@ -378,6 +378,27 @@ int
> > link_config_apply(link_config_ctx *ctx, link_config *config, struct
> udev_dev
> > return -ENODEV;
> > }
> >
> > + switch (config->mac_policy) {
> > + case MACPOLICY_PERSISTENT:
> > + if (!mac_is_permanent(device)) {
> > + r = get_mac(device, false, &generated_mac);
> > + if (r < 0)
> > + return r;
> > + mac = &generated_mac;
> > + }
> > + break;
> > + case MACPOLICY_RANDOM:
> > + if (!mac_is_random(device)) {
> > + r = get_mac(device, true, &generated_mac);
> > + if (r < 0)
> > + return r;
> > + mac = &generated_mac;
> > + }
> > + break;
> > + default:
> > + mac = config->mac;
> > + }
> > +
> > if (ctx->enable_name_policy && config->name_policy) {
> > NamePolicy *policy;
> >
> > @@ -394,6 +415,18 @@ int link_config_apply(link_config_ctx *ctx,
> link_config *config, struct udev_dev
> > break;
> > case NAMEPOLICY_MAC:
> > new_name =
> > udev_device_get_property_value(device, "ID_NET_NAME_MAC");
> > + if (mac && new_name) {
> > + char *mac_name = NULL;
> > + mac_name =
> > strdup(new_name);
> > + if (!mac_name)
> > + break;
> > +
> > snprintf(mac_name+strlen(mac_name)-
> (2*ETHER_ADDR_LEN), 2*ETHER_ADDR_LEN+1,
> > +
> > "%02x%02x%02x%02x%02x%02x",
> > +
> > mac->ether_addr_octet[0], mac-
> >ether_addr_octet[1], mac->ether_addr_octet[2],
> > +
> > mac->ether_addr_octet[3], mac-
> >ether_addr_octet[4], mac->ether_addr_octet[5]);
> > + new_name = mac_name;
> > + *free_name = true;
> > + }
> > break;
> > default:
> > break; @@ -408,27 +441,6 @@
> > int link_config_apply(link_config_ctx *ctx, link_config *config, struct
> udev_dev
> > else
> > *name = NULL;
> >
> > - switch (config->mac_policy) {
> > - case MACPOLICY_PERSISTENT:
> > - if (!mac_is_permanent(device)) {
> > - r = get_mac(device, false, &generated_mac);
> > - if (r < 0)
> > - return r;
> > - mac = &generated_mac;
> > - }
> > - break;
> > - case MACPOLICY_RANDOM:
> > - if (!mac_is_random(device)) {
> > - r = get_mac(device, true, &generated_mac);
> > - if (r < 0)
> > - return r;
> > - mac = &generated_mac;
> > - }
> > - break;
> > - default:
> > - mac = config->mac;
> > - }
> > -
> > r = rtnl_set_link_properties(ctx->rtnl, ifindex, config->alias,
> > mac,
> config->mtu);
> > if (r < 0) {
> > log_warning("Could not set Alias, MACAddress or MTU
> > on %s: %s", old_name, strerror(-r)); diff --git
> > a/src/udev/net/link-config.h b/src/udev/net/link-config.h index
> > a55c6f5..460501c 100644
> > --- a/src/udev/net/link-config.h
> > +++ b/src/udev/net/link-config.h
> > @@ -75,7 +75,7 @@ int link_config_load(link_config_ctx *ctx); bool
> > link_config_should_reload(link_config_ctx *ctx);
> >
> > int link_config_get(link_config_ctx *ctx, struct udev_device *device,
> > struct link_config **ret); -int link_config_apply(link_config_ctx
> > *ctx, struct link_config *config, struct udev_device *device, const
> > char **name);
> > +int link_config_apply(link_config_ctx *ctx, struct link_config
> > +*config, struct udev_device *device, const char **name, bool
> > +*free_name);
> >
> > const char *name_policy_to_string(NamePolicy p) _const_; NamePolicy
> > name_policy_from_string(const char *p) _pure_; diff --git
> > a/src/udev/udev-builtin-net_setup_link.c
> > b/src/udev/udev-builtin-net_setup_link.c
> > index b7ba8c9..7c8346b 100644
> > --- a/src/udev/udev-builtin-net_setup_link.c
> > +++ b/src/udev/udev-builtin-net_setup_link.c
> > @@ -27,6 +27,7 @@ static link_config_ctx *ctx = NULL;
> >
> > static int builtin_net_setup_link(struct udev_device *dev, int argc, char
> **argv, bool test) {
> > const char *name;
> > + bool free_name = false;
> > link_config *link;
> > int r;
> >
> > @@ -46,14 +47,17 @@ static int builtin_net_setup_link(struct udev_device
> *dev, int argc, char **argv
> > }
> > }
> >
> > - r = link_config_apply(ctx, link, dev, &name);
> > + r = link_config_apply(ctx, link, dev, &name, &free_name);
> > if (r < 0) {
> > log_error("Could not apply link config to %s",
> udev_device_get_sysname(dev));
> > return EXIT_FAILURE;
> > }
> >
> > - if (name)
> > + if (name) {
> > udev_builtin_add_property(dev, test, "ID_NET_NAME",
> > name);
> > + if (free_name)
> > + free(name);
> > + }
> >
> > return EXIT_SUCCESS;
> > }
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > systemd-devel mailing list
> > systemd-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
