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

cmd/snap-update-ns: add compare function for mount entries #2848

Merged
merged 1 commit into from Feb 23, 2017

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Feb 14, 2017

This patch adds a simple function for comparing mount entries. This will
be followed up with a simple routine that sorts two lists of mount
entries quickly.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

This patch adds a simple function for comparing mount entries. This will
be followed up with a simple routine that sorts two lists of mount
entries qucikly.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@zyga zyga requested review from jdstrand and removed request for jdstrand February 17, 2017 13:58
Copy link
Contributor

@chipaca chipaca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one question about fields being NULL, which is only a blocker if it's got the wrong answer :-)

@@ -31,6 +31,16 @@
#include "../libsnap-confine-private/cleanup-funcs.h"

/**
* Compare two mount entries (through indirect pointers).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a "(needed for using from qsort)" to the comment so people don't scratch their heads as I did

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is static and it will show up in the next branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, without further context the need for this is awkward indeed.

{
int result;
if (a == NULL || b == NULL) {
die("cannot compare NULL mount entry");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not even if they're both NULL :-p?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we never have NULL entries that mean anything. We only have an empty list that is expressed by an NULL entry. The sorting routine will not compare NULL pointers.

if (a == NULL || b == NULL) {
die("cannot compare NULL mount entry");
}
result = strcmp(a->entry.mnt_fsname, b->entry.mnt_fsname);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how sure are we that all these fields are non-null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% sure, we copy those with an explicit check that converts NULL to ""

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of details for follow ups. Merging now nevertheless.

*
* Returns 0 if both entries are equal, a number less than zero if the first
* entry sorts before the second entry or a number greater than zero if the
* second entry sorts before the second entry.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

End of the sentence has two "second entry".

@@ -31,6 +31,16 @@
#include "../libsnap-confine-private/cleanup-funcs.h"

/**
* Compare two mount entries (through indirect pointers).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, without further context the need for this is awkward indeed.

@niemeyer niemeyer merged commit bc7549d into snapcore:master Feb 23, 2017
@zyga zyga deleted the mount-entry-compare branch August 22, 2017 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants