smack-team / smack

Smack userspace
GNU Lesser General Public License v2.1
41 stars 33 forks source link

Merge common pieces of code #73

Closed rafal-krypa closed 10 years ago

rafal-krypa commented 11 years ago

Refactoring repeated pieces of code by moving it to internal functions. Merged pieces of the following function pairs:

Three patches makes the code shorter by 128 lines total. More importantly modifications to these parts will have to be done in single place, which should improve maintainability.

rafal-krypa commented 10 years ago

This has merged very badly and actually requires a rewrite to work with #53. Please revert this patch for now.

jarkkojs commented 10 years ago

OK, what can you do, this sometimes happens even though I thought I tested it properly :( No worries, I'll revert it and tag v1.0.4 so that we can pull that to tizen. I'll take a revised version of this after I've done v1.0.4.

jarkkojs commented 10 years ago

@rafal-krypa, maybe in situations like this we could use tag like v1.0.3.1. Only in situations where invalid patch slips into release. That kind of documents it. Does that work for you?

jarkkojs commented 10 years ago

@rafal-krypa, commit is now reverted. Should I tag v1.0.3.1 now? Can you do a quick sanity check on v1.0.x branch.

rafal-krypa commented 10 years ago

I have rewritten the commit and added two more in the same manner. These two were inspired by experiments with rule merging. I found that every modification to smack_accesses data structure required similar changes in several places. The issue title and description is now updated to reflect these additions.

jarkkojs commented 10 years ago

I've been thinking that this change should be eventually made (use accesses_apply for both cases) so this does work for me.

Some comments:

rafal-krypa commented 10 years ago

I've introduced function accesses_print() and call it from accesses_apply() and smack_accesses_save(). The latter is now a simple wrapper for accesses_print(), but the former is still handling file opening and fallback from load2 to load. If you prefer some other name than accesses_print, I have no problem with changing that. But accesses_apply is already taken.

jarkkojs commented 10 years ago

You should be able squeeze that patch to a smaller change by putting accesses_print declaration after accesses_apply declaration. Now the patch has unnecessary clutter because place of accesses_apply is swapped.

When writing to a normal file there's no guarantee that write() will write the number of bytes given in the count parameter when writing rules to a file. You should have a loop that would call write has written all the bytes..

jarkkojs commented 10 years ago

Looks great now!