Merge lp://staging/~louis/ubuntu/trusty/clamav/lp799623_fix_logrotate into lp://staging/ubuntu/trusty/clamav

Proposed by Louis Bouchard
Status: Merged
Merge reported by: Martin Pitt
Merged at revision: not available
Proposed branch: lp://staging/~louis/ubuntu/trusty/clamav/lp799623_fix_logrotate
Merge into: lp://staging/ubuntu/trusty/clamav
Diff against target: 28 lines (+9/-1)
2 files modified
debian/changelog (+8/-0)
debian/common_functions (+1/-1)
To merge this branch: bzr merge lp://staging/~louis/ubuntu/trusty/clamav/lp799623_fix_logrotate
Reviewer Review Type Date Requested Status
Martin Pitt Approve
Review via email: mp+205627@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pitt (pitti) wrote :

This looks like it could easily introduce a regression, especially on config files which look "normal", i. e. only use one space to separate words. Are you sure that instead of

  value=`grep ^$variable[[:space:]] $CLAMAVCONF | head -n1 | awk '{print $2}'`

you don't mean

  value=`grep ^$variable[[:space:]]* $CLAMAVCONF | head -n1 | awk '{print $2}'`

?

review: Needs Fixing
Revision history for this message
Martin Pitt (pitti) wrote :

Oh, the $CLAMAVCONF is not part of the search string indeed. This really ought to be quoted, like

value=`grep "^$variable " $CLAMAVCONF | head -n1 | awk '{print $2}'`

Revision history for this message
Martin Pitt (pitti) :
review: Approve
Revision history for this message
Colin Watson (cjwatson) wrote :

On Wed, Feb 12, 2014 at 03:12:52PM -0000, Martin Pitt wrote:
> value=`grep ^$variable[[:space:]]* $CLAMAVCONF | head -n1 | awk '{print $2}'`

Also surely that regex should be double-quoted? Otherwise that might
happen to expand to something in the current working directory and weird
stuff would ensue.

Revision history for this message
Louis Bouchard (louis) wrote :

Makes perfect sense to me. I was too trusty of the patch proposed in the bug.

I will rework according to the suggestions

Revision history for this message
Martin Pitt (pitti) wrote :

Uploaded with

- value=`grep ^$variable $CLAMAVCONF | head -n1 | awk '{print $2}'`
+ value=`grep "^$variable[[:space:]]" $CLAMAVCONF | head -n1 | awk '{print $2}'`

now. I didn't actually use UDD, that branch takes abysmally long to check out (I killed it after 10 mins or so).

Thanks!

Revision history for this message
Louis Bouchard (louis) wrote :

Before I go any further, the addition of [[:space:]] was to discriminate on word boundary. When the CLAMAVCONF file has entries like this :

StreamMaxLength 25M
LogFileMaxSize 0
LogFile /var/log/clamav/clamav.log

the grep could not discriminate b/w LogFileMaxSize & LogFile, so the LogFile variable ended up with 0.

Revisiting the expression, I think that using \b would be more appropriate to discriminate on word boudaries so the expression would be :

value=`grep "^$variable\b" $CLAMAVCONF | head -n1 | awk '{print $2}'`

Unless you see an issue with this, I will go ahead with that fix, since the [[:space:]] was not intended to identify single space, but word boundaries

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches