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

Allow mfultralight and mfc tools to work also with specific UID's #350

Merged
merged 7 commits into from
May 15, 2016
Merged

Conversation

oliv3r
Copy link
Contributor

@oliv3r oliv3r commented May 13, 2016

This patch series does some code cleanup/fixups on a small scale, but most importantly adds the ability to specify UID's to mfc-classic and mfultralight tools to r/w a specific tag when multiple tags are insight.

Currently the check on argc is done twice, once in each if branch. This
is silly and we can just check once and fail right away.

Signed-off-by: Olliver Schinagl <[email protected]>
The nfc-mfclassic utility will pick a seemingly random (the libnfc
default which seems to be the lowest UID). With the new (u|U) options
it is now possible to force a UID and thus write a specific tag, which
can be very useful if there are more then one tag visible.

Signed-off-by: Olliver Schinagl <[email protected]>
For some reason, 0cece94 changed the argc count check to only show
the help if argc is 0. Obviously, argc is never zero, as the first
argument in argv is always the binary itself. Revert that and show usage
if there is no arguments supplied to the binary.

Signed-off-by: Olliver Schinagl <[email protected]>
When several tags are in range, nfc-mfultralight uses the lowest ID it
finds by default. This patch adds some code from nfc-list that lists the
tags in range whenever an operation is performed (r/w). Further more it
adds the --with-uid <UID> option to force reading/writing of a specific
tag.

The UID can be up to 10 bytes long and can be optionally separated by
colons or hyphens (MAC address style).

Signed-off-by: Olliver Schinagl <[email protected]>
Clean up some stray unintended whitespaces. This patch does not
introduce any binary changes.

Signed-off-by: Olliver Schinagl <[email protected]>
We use a variable, uiBlocks, to determine how many blocks to read/write.
Reading is actually done via a hardcoded 0xF value however.

Additionally, make uiblocks a const, as we use it as a constant and
change the page variable to uint32_t for consistency sake.

Signed-off-by: Olliver Schinagl <[email protected]>
Currently, we return false, and after turning set the bFailure state.
This is of course not possible.

Signed-off-by: Olliver Schinagl <[email protected]>
@neomilium neomilium merged commit 403650a into nfc-tools:master May 15, 2016
@neomilium
Copy link
Member

@oliv3r : Good job, thanks ! 👍

@quantum-x
Copy link
Contributor

quantum-x commented Feb 27, 2017

@neomilium This is probably way too late, but it is worth noting that this patch breaks backward compatibility by adding the (unnecessary) u switch.

I propose that u/U is unnecessary, defaulting to 'standard' behavious but can be provided if required.
libnfc is used by many people in many different scenarios, shouldn't patches that break compatibility should be discussed before they're merged ?

It seems like this patch is a 'convenience' patch by @oliv3r - while it's a useful contribution, it shouldn't be enforced on everyone else.

@doegox
Copy link
Member

doegox commented Feb 27, 2017

@quantum-x we try to be careful regarding the libnfc itself and its API, but in our opinion the provided tools are just (useful) examples based on the libnfc and their CLI usage must not be preserved at all cost.

E.g. if you want to provide a more future-proof interface with single-letter Unix-style options + their GNU-style long-named equivalents, you're more than welcome to submit a patch.

@quantum-x
Copy link
Contributor

@doegox No arguments from me based off your points.
With that said, the submitted patch introduces an optional functionality, seemily a convenience function for the author. They're definitely flags that could be made optional.

If we don't need to break CLI compatibility, why do it?
Other contributors (myself included, if you look at my patch history) are able to provide non-breaking flags.

@doegox
Copy link
Member

doegox commented Feb 27, 2017

The current scheme to pass arguments to nfc-mfclassic is a list of fixed fields so if you want to add an option you have to add a field. Only last field(s) can be optional and this is already the case for [<keys.mfd> [f]]. The only proper way to avoid that is to pass optional arguments as most CLI do, this is what I proposed in my answer.

@quantum-x
Copy link
Contributor

Seems like a great idea.
Perhaps that requirement should have been / should be part of the original PR to avoid headaches for other users.

@oliv3r is this something that you would consider implementing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants