cmd/snap-update-ns: compute next action to transition mount profile

[zyga]

This patch adds a function that given two sorted mount profiles (current
and desired) computes the next action that needs to be taken to
transition the current profile to the desired profile.

The function operates on a simple principle. Identical entires are
skipped. Entries that don't show up in the desired profile but exist in
the current profile are umounted. Entries that are in the desired
profile but are not in the current profile are mounted.

The logic ensures that we always unmount everything that should be
unmounted before proceeding with the first thing to be mounted.

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

[jdstrand]

gotos can be quite useful and the correct choice for jumping forward to error/cleanup/exit code, but I'd prefer a proper loop here instead of jumping backward via the goto.

[zyga]

I'll look at re-doing this, thanks!

[jdstrand]

'to all the there'

[zyga]

@jdstrand apart from mechanical changes is the algorithm correct?

[zyga]

Please have a 2nd look on this.

[stolowski]

I'm not sure if I got it right - it seems to me that sc_compute_required_mount_changes main loop processes a (desired, current) pair on each iteration, so if we have e.g.

desired:
A
B

current:
B
C

it will unmount B on 1st iteration, but then mount it back on 2nd iteration even if B mount entry is identical in both? In other words we would skip identical entires only if they had same positions in boths lists, or did I misunderstand it?

[zyga]

I need to re-think some things. The comment from Paweł made me realise there are some deeper problems here.

[zyga]

@stolowski I've corrected this issue and added a test case for it. Please have a 2nd look.

[jdstrand]

I desk checked this and assuming I know how sc_compute_required_mount_changes() is intended to be called, it appears correct. However, this is extremely complicated with the loop in sc_compute_required_mount_changes() and the expected loop that calls sc_compute_required_mount_changes(). I think it would be much cleaner if instead you called sc_compute_required_mount_changes() once and it build up a list of sc_mount_change then iterate on that list to perform the mounts and unmounts.

Eg, something like this psuedo-code:

// changes is a linked list of sc_mount_change where sc_compute_required_mount_changes
// builds up this list with a single invocation
sc_compute_required_mount_changes(desired, current, changes);

// now that changes has stuff in it, perform the operations
struct sc_mount_change *change = changes;
if change != NULL:
   while change != NULL:
      if change.action == SC_ACTION_UNMOUNT:
        umount(change.entry)
      elif change.action == SC_ACTION_UNMOUNT:
        mount(change.entry)
      change = change.next;

  deallocate what was allocated in changes

then write sc_compute_required_mount_changes() such that it does two passes-- one for mounts and one for unmounts. Eg in pseudo-code:

bool find(haystack, needle) { ... }
void sc_compute_required_mount_changes(desired, current, changes) {
    // first see what to unmount
    c = current;
    while c != NULL:
      if find(desired, c) == False:
        change = allocate for sc_mount_change
        change.entry = copy(c)
        change.action = SC_ACTION_UNMOUNT
        change.next = NULL;
        c = c->next;
        append change to changes

    // then see what to mount
    d = desired;
    while d != NULL:
      if find(current, d) == False:
        change = allocate for sc_mount_change
        change.entry = copy(d)
        change.action = SC_ACTION_MOUNT
        change.next = NULL;
        d = d->next;
        append change to changes
}

These loops are much easier to read IME and easier to verify. Please consider slightly longer tests like the following too (non-exhaustive):

desired        current        expected
BD             ACE            BD
ACE            BD             ACE
ABDEG          BCDE           ABDEG
BCDE           ABDEG          BCDE

[jdstrand]

It would be nice to have a comment here on when again is expected to by used and how it relates to flag

[zyga]

Again was removed so this is no longer an issue.

[jdstrand]

This could simply be: } else { // c != NULL && d != NULL

[zyga]

Yes but I'd rather be explicit about it.

[jdstrand]

Since the continue is above, you could avoid the indentation here by simply doing:

    ...
    continue;
}
// entries are not equal which means that we need to unmount the current
// entry and mount the desired entry.
...

[zyga]

I've simplified the code now.

[jdstrand]

You aren't managing change->entry->next here. I guess how this is meant to be used is:

loop on:
  sc_compute_required_mount_changes(...)
  if change:
    sc_do_some_umount_thing(change)

[zyga]

entry->next is only used to combine entries into a list. It has no purpose for this part of the code.

[stolowski]

Thanks for adding the handling of the case I questioned earlier. Looks good overall (modulo Jamie's comments). Just one nitpick re flag plus a general question: apparently sc_mount_entry_find introduces quadratic complexity - what kind of inputs do we expect here (how many entries? Do we expect the lists to be "misaligned" -so to speak- a lot so that sc_mount_entry_find kicks in very often?)?. Is this kind of complexity something we should care about here. If so, then perhaps a lookup map would be better than scanning the list every time, but I'm not going to push for premature optimization if this is not really justified.

[stolowski]

If this is a general purpose-purpose flag (with a prospect of holding more states in the future), then I'd add a const (#define) for the possible states (just one state for now) and make sure if (d->flag) checks for the exact state. Alternatively, if we don't expect multiple states, then I'd rename "flag" to clearly indicate what is it about.

[mvo5]

If this is used for more in the future, maybe we can change this code here once this will actually be changed in the future? YAGNI and all :) ? Or do we know already that it will change and in what way?

[mvo5]

Some comments, would be great if those could be addressed. Overall I think its fine.

[mvo5]

Wonder if we can have a test_entry_3 here? I found it slightly confusing why this is different than the other tests.

[mvo5]

Maybe a small comment what this function is doing (in a high level sense). Like:

def sc_compute_required_mount_changes(desired_set, current_set):
    res = []
    for entry in current-desired:
        res.append((entry, "unmount"))
    for entry in desired-current:
        res.append((entry, "mount"))
    return res

[mvo5]

The flag feels overly generic. Maybe something like seen,processed,considered,present_in_both,equal.

[stolowski]

If this is a general purpose-purpose flag (with a prospect of holding more states in the future), then I'd add a const (#define) for the possible states (just one state for now) and make sure if (d->flag) checks for the exact state. Alternatively, if we don't expect multiple states, then I'd rename "flag" to clearly indicate what is it about.

[mvo5]

If this is used for more in the future, maybe we can change this code here once this will actually be changed in the future? YAGNI and all :) ? Or do we know already that it will change and in what way?

[zyga]

I found another issue in this. I'll close the PR for a moment and push a fix and a new test.