smack-team/smack

Regression in eaf908fd

Closed this issue · 11 comments

jarkkojs commented

smack_accesses_apply should leave rules intact. However it modifies access_type of a rule while looping through them. This regression was introduced by eaf908f.

rafal-krypa commented

I will look at this in few hours.

jarkkojs commented

Thanks for quick reaction! BTW, when you do the fix I would suggest that you fill the buffer simply like:

memset(access_type, '-', 5);
access_type[5] = 0;

Simple, stupid, safe and efficient..

rafal-krypa commented

How would you like this patch: https://github.com/rafal-krypa/smack/commit/74f5b5c21a2ed9c0576c3c3ab3802d11d66098a9?
I cannot test it as I'm out of office for this whole week, but the patch is small enough.

I don't want to make this fix too fancy because I'd like to suggest greater rework of that function later.

jarkkojs commented

Thanks, I'll test this on Monday.

jarkkojs commented

Removed my previous comment. This is exactly right thing to do! I don't what I was thinking in the morning.. Merging soon!

jarkkojs commented

Your commit has merge conflicts. Can you fix them and attach your pull request here as explained in #33? Thanks.

jarkkojs commented

Hmm.. I can fix those. Nothing too complicated.

jarkkojs commented

Actually, I would appreciate if you would rebase your commit. It does cause quite many merge conflicts. And maybe it would make sense to turn this into a proper pull request. I want this fix to be fancy because it is going to a release. New features are always lower priority than proper bug fixes.

rafal-krypa commented

I'm afraid that I cannot attach a pull request here. The hub tool gives me this error:

Error creating pull request: Unprocessable Entity (HTTP 422)
Not allowed to change field "issue"
rafal-krypa commented

It seems to be a known feature of the tool: mislav/hub#332

jarkkojs commented

Yes. You have to create a remote branch for "to be merged" commits.