Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Plugin for validating MAC-Addresses #2621

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@e01306821
Copy link
Contributor

commented Apr 10, 2019

WIP for CM2019-homework.

This plugin validates MAC-addresses. The following MAC-address-formats are allowed for now:

  • XX-XX-XX-XX-XX-XX
  • XX:XX:XX:XX:XX:XX
  • XXXXXX-XXXXXX

If "set" is used, the value will be transformed to the format of the middle one.

TODO:

  • Write tests
  • Update readme
  • Better error-messages
  • Maybe also support formats "XXXXXXXXXXXX" and "XXXX.XXXX.XXXX"
@markus2330
Copy link
Contributor

left a comment

Thank you for the PR!


## Introduction

Copy this macaddr if you want to start a new

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Please copy the PR description here so that it is easier to give feedback on it.

status = implemented
type = string
usedby/plugins = macaddr
description = TODO

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Should describe what the metadata does.

#include <kdbprivate.h>
#include <stdio.h>

void insertSeperator (char * mac)

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

can be static

}
else if (sepOcc == 1)
{
char * newmac = (char *) malloc (strlen (mac) + 4);

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Please use elektraMalloc (and do not forget to call elektraFree).

}
}

if (sepOcc == 5)

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Avoid magic constants. Give it some name or at least write a comment why it is 5.

return ELEKTRA_PLUGIN_STATUS_SUCCESS;
}

int elektraMacaddrClose (Plugin * handle ELEKTRA_UNUSED, Key * errorKey ELEKTRA_UNUSED)

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

If it is not needed, you can remove these functions.


succeed_if (plugin->kdbError (plugin, ks, parentKey) == ELEKTRA_PLUGIN_STATUS_SUCCESS, "call to kdbError was not successful");

succeed_if (plugin->kdbClose (plugin, parentKey) == ELEKTRA_PLUGIN_STATUS_SUCCESS, "call to kdbClose was not successful");

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Please add real tests. Maybe shell recorder tests are better suited. See tests/shell/shell_recorder/tutorial_wrapper/README.md

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

You should also transform the MAC-address in kdbGet and in this case probably also restore to the original format in kdbSet. You can look at the boolean and enum parts of the type plugin, to get an idea of how such a behaviour can be implemented.

It would also be nice, if the plugin could transform the MAC-address into a decimal number. Then the high-level API could more easily be used to read the value (as a 64 bit integer).

@e01306821

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Thanks for your review!
@kodebach You mean transform the address in kdbGet instead of kdbSet?
Or should I rather return a 64 bit integer on kdbGet instead of returning a "standardized" MAC-string? If yes just so that I am understanding it correctly, using kdbGet shall return an integer-MAC instead of a "conventional" MAC-address (using my supported formats as checks if the read address is valid) and kdbSet shall check and transform an integer-value back to the original format if there was any (if not, then one of my supported formats will be written)?

@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Probably @markus2330 should decide what the standard format for MAC addresses will be. MAC addresses are 48 bits long, so a 64 bit integer could store them. Which could then be retrieved using elektraGetUnsignedLongLong.

The transformation should be done in kdbGet. In addition you store the original value in the origvalue metakey. Then in kdbSet you transform any key that is marked as a MAC address (check/macaddr set), but not those with origvalue. If the key has origvalue, it wasn't changed (ensured by the framework) so you simply set the key to the original value and remove origvalue. The keys that were transformed are immediately restored to there original value as well (you can also just do the format check and skip the transformation/restore if possible).

The point of doing this is that the user always sees their preferred representation (since we always restore) and the program using Elektra always sees the standard form.

One additional note: Right now only one plugin can normalize a keys value, so if for some reason origvalue is already set, you shouldn't change it or the keys value. Instead you should raise an error in this case, noting the multiple plugins tried to normalize the key.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Yes, 48bit in a 64bit integer sounds fine.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

This pull request introduces 1 alert when merging 42eb9cb into 27b31c2 - view on LGTM.com

new alerts:

  • 1 for Returning stack-allocated memory

Comment posted by LGTM.com

e01306821 added some commits Apr 6, 2019

Updated doc
Still incomplete
WIP: Addresses are returned as integer
... but I don't know yet why tests fail, locally everything works
Added WIP release notes
Test push to see if test also fails on server (running all commands locally works without problems, running them inside a test for some reason not)

@e01306821 e01306821 force-pushed the e01306821:plugin_mac branch from 42eb9cb to 224611b May 21, 2019

e01306821 added some commits May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.