I think, nm_utils_hwaddr_type() should accept unknown lengths and just return "-1". It is useful for functions with a "is_a" or "has_a" semantic to allow invalid types, so that they can be used to check for valid types.

I pushed a commit on top of yours changing this behaviour (and reverting your change). I also checked all uses of those functions and related hwaddr functions that they behave reasonable in case of errors and/or assert against the input.

Your test case looks good to me.

We deprecated nm_utils_hwaddr_type() though:

 * Deprecated: This could not be extended to cover other types, since
 * there is not a one-to-one mapping between types and lengths. This
 * was mostly only used to get a type to pass to
 * nm_utils_hwaddr_ntoa() or nm_utils_hwaddr_aton() when you only had
 * a length; but you can just use nm_utils_hwaddr_ntoa_len() or
 * nm_utils_hwaddr_aton_len() now instead.

So perhaps we should switch to nm_utils_hwaddr_atoba_len() for the first case?  At least if we're here, we should rewrite the code to stop using deprecated functions.

---

Thomas' change looks fine to me.

(In reply to comment #2)
> We deprecated nm_utils_hwaddr_type() though:
> 
>  * Deprecated: This could not be extended to cover other types, since
>  * there is not a one-to-one mapping between types and lengths. This
>  * was mostly only used to get a type to pass to
>  * nm_utils_hwaddr_ntoa() or nm_utils_hwaddr_aton() when you only had
>  * a length; but you can just use nm_utils_hwaddr_ntoa_len() or
>  * nm_utils_hwaddr_aton_len() now instead.
> 
> So perhaps we should switch to nm_utils_hwaddr_atoba_len() for the first case? 
> At least if we're here, we should rewrite the code to stop using deprecated
> functions.

I think that is not possible, because we use the length for guessing the type of the MAC address. There is not replacement for that.

- nm_utils_hwaddr_ntoa() or nm_utils_hwaddr_aton():
  already expect the "type", which nm_utils_hwaddr_type() guesses.

- nm_utils_hwaddr_ntoa_len():
  expects that you already know the length (and indirectly the type).
  It will then check that the provided input string parses as a valid MAC
  address of that type/length.


A better name for nm_utils_hwaddr_type() would be nm_utils_guess_hwaddr_type_from_len()

I pushed 4 additional commits.

>> libnm-util: undeprecated nm_utils_hwaddr_type()

It seems useful to be able to test for common HWADDR lengths. Maybe we should not deprecate nm_utils_hwaddr_type() -- ?


>> keyfile: don't check HWADDR length in mac_address_parser()
>> keyfile: don't check HWADDR length in mac_address_writer()
>> cli: don't use nm_utils_hwaddr_type() to stringify HWADDR

no longer user nm_utils_hwaddr_type(). If we keep the function deprecated, we should not use it. Even if we decide to undeprecated it, we might want to keep these patches, because they make the parsing/stringification of HWADDR more relaxed. Which *could* be desired -- ?

(In reply to comment #4)
> I pushed 4 additional commits.
> 
> >> libnm-util: undeprecated nm_utils_hwaddr_type()
> 
> It seems useful to be able to test for common HWADDR lengths. Maybe we should
> not deprecate nm_utils_hwaddr_type() -- ?

But it can never be made much more generic, because most of the ARPHRD types have length 8. So it's really "distinguish between ARPHRD_ETHER and ARPHRD_INFINIBAND but only if you know it has to be one or the other". And that's pretty easy to open code.

> >> keyfile: don't check HWADDR length in mac_address_parser()

An alternative here that wouldn't lose any checking would be to add two wrappers around mac_address_parser() that call into the same generic function, passing the expected length along with the other args. (And likewise with writer.)

(In reply to comment #5)
> (In reply to comment #4)
> > I pushed 4 additional commits.
> > 
> > >> libnm-util: undeprecated nm_utils_hwaddr_type()
> > 
> > It seems useful to be able to test for common HWADDR lengths. Maybe we should
> > not deprecate nm_utils_hwaddr_type() -- ?
> 
> But it can never be made much more generic, because most of the ARPHRD types
> have length 8. So it's really "distinguish between ARPHRD_ETHER and
> ARPHRD_INFINIBAND but only if you know it has to be one or the other". And
> that's pretty easy to open code.
> 
> > >> keyfile: don't check HWADDR length in mac_address_parser()
> 
> An alternative here that wouldn't lose any checking would be to add two
> wrappers around mac_address_parser() that call into the same generic function,
> passing the expected length along with the other args. (And likewise with
> writer.)

Yeah, these 4 patches are suggestions, not sure we want to do that (e.g. the un-deprecation?).

I pushed a fixup! with that suggestion. Not sure if it's a good idea though. There is already NMSetting:verify() which enforces the proper length of the MAC address. This kind of duplicates this requirement. On the other hand, it moves the validation closer to reading the keyfile value, so it might be better. Opinions?

(In reply to comment #6)
> There is already NMSetting:verify() which enforces the proper length of the MAC
> address. This kind of duplicates this requirement. On the other hand, it moves
> the validation closer to reading the keyfile value, so it might be better.
> Opinions?

the error from NMSetting:verify() includes the property name that it's talking about, so it should still be pretty clear what the problem is at that point.

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I pushed 4 additional commits.
> > > 
> > > >> libnm-util: undeprecated nm_utils_hwaddr_type()
> > > 
> > > It seems useful to be able to test for common HWADDR lengths. Maybe we should
> > > not deprecate nm_utils_hwaddr_type() -- ?
> > 
> > But it can never be made much more generic, because most of the ARPHRD types
> > have length 8. So it's really "distinguish between ARPHRD_ETHER and
> > ARPHRD_INFINIBAND but only if you know it has to be one or the other". And
> > that's pretty easy to open code.
> > 
> > > >> keyfile: don't check HWADDR length in mac_address_parser()
> > 
> > An alternative here that wouldn't lose any checking would be to add two
> > wrappers around mac_address_parser() that call into the same generic function,
> > passing the expected length along with the other args. (And likewise with
> > writer.)
> 
> Yeah, these 4 patches are suggestions, not sure we want to do that (e.g. the
> un-deprecation?).
> 
> I pushed a fixup! with that suggestion. Not sure if it's a good idea though.
> There is already NMSetting:verify() which enforces the proper length of the MAC
> address. This kind of duplicates this requirement. On the other hand, it moves
> the validation closer to reading the keyfile value, so it might be better.
> Opinions?

I like this approach.  The squashed branch looks good to me and passes 'make check'.

Branch merged as:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=b5a94ae4796dac2cc757d301a27e1a6f91ee23a3


(dropped patch with undeprecation nm_utils_hwaddr_type())