On Fri, Jun 1, 2018 at 9:56 PM, Stephen Hemminger
<step...@networkplumber.org> wrote:
> On Tue, 29 May 2018 16:57:07 +0200
> Patrick Talbert <ptalb...@redhat.com> wrote:
>
>> As mentioned in the ip-address man page, an address label must
>> be equal to the device name or prefixed by the device name
>> followed by a colon. Currently the only check on this input is
>> to see if the device name appears at the beginning of the label
>> string.
>>
>> This commit adds an additional check to ensure label == dev or
>> continues with a colon.
>>
>> Signed-off-by: Patrick Talbert <ptalb...@redhat.com>
>> Suggested-by: Stephen Hemminger <step...@networkplumber.org>
>
> Yes, this looks better but still have some feedback.
>
>> ---
>> ip/ipaddress.c | 21 +++++++++++++++++++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 00da14c..fce2008 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>> return false;
>> }
>>
>> +static bool is_valid_label(const char *dev, const char *label)
>> +{
>> + char alias[strlen(dev) + 1];
>> +
>> + if (strlen(label) < strlen(dev))
>> + return false;
>> +
>> + strcpy(alias, dev);
>> + strcat(alias, ":");
>> + if (strncmp(label, dev, strlen(dev)) == 0 ||
>> + strncmp(label, alias, strlen(alias)) == 0)
>> + return true;
>> + else
>> + return false;
>> +}
>
> This string copying and comparison still is much more overhead than it
> needs to be. The following tests out to be equivalent with a single strncmp
> and strlen.
>
> Why not just:
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 00da14c6f97c..eac489e94fe4 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -2040,6 +2040,16 @@ static bool ipaddr_is_multicast(inet_prefix *a)
> return false;
> }
>
> +static bool is_valid_label(const char *label, const char *dev)
> +{
> + size_t len = strlen(dev);
> +
> + if (strncmp(label, dev, len) != 0)
> + return false;
> +
> + return label[len] == '\0' || label[len] == ':';
> +}
> +
Woah. This is way better. v3 coming up....
Thank you for all of your help with this... and by help I mean writing
the patch.
>
>
> Doesn't matter much now, but code seems to get copied.
