Mailing List Archive

Re: PATCH: Enhanced exclude / include support for clamscan
Thanks for the feedback. Please forgive the formatting of this response; I am unfortunately stuck using Outlook :-/, and I'm sure it's going to stick line breaks in the wrong places.

>While these features might make a lot of sense, I can't help thinking
>that this isn't the 'Unix' (right:) way to go about implementing them.

I've been using Unix for over two decades and have been known to edit raw sendmail CF files, so I think I know a thing or two about the "Unix way" :-).

On the one hand, you're right that were clamscan to have a command-line option to tell it to read a list of files on stdin, the logic I built into it could be implemented externally, either with home-grown scripts or with a separate utility bundled with clamav. I certainly wouldn't be adverse to achieving my goal that way, where that functionality to be provided by clamscan.

On the other hand, I don't quite view clamscan as being a pipeline tool of the same genre as sed, tr, etc. I think of it as a full-fledged application, and as such, I think it's reasonable for this kind of functionality to be built into it if it's something that a lot of people are going to find useful.

Other applications in the same genre with similar functionality include rsync, rdiff-backup, and GNU tar. All of those, like clamscan, could force the user to implement exclude / include functionality externally and then feed a file list into the application, and yet all of them provide complex, flexible built-in filtering functionality of their own.

Perhaps what is needed is a compromise -- basic filtering of the sort I implemented, *plus* the "-f" flag (or the equivalent) for people who want to roll their own logic?

It is also worth noting that I was not adding new functionality to the application but rather enhancing existing functionality. If there had been no --exclude and --include flags in clamscan to begin with, I probably would have implemented something like what you propose. But given that they were there and buggy (recompiling all the regexps for every file / directory being considered), I felt that fixing / enhancing the existing functionality was the right way to go.

>You mentioned maintenance. IMO, what you're doing is asking for a
>maintenance headache. You mentioned changing the API. Please, for
>the sake of people that are using the package, already, don't do that
>unless there is (a) a VERY compelling reason and (b) NO other way.

Interfaces change. Such is life. Remaining wedded to broken interfaces leads over time to difficult to maintain code with inferior functionality and performance. The changes should be visibly documented, and people who are using ClamAV should be smart enough to read the release notes when they upgrade.

If GNU tar, a far more widely used utility than ClamAV, can get away with changing its interface as frequently as it does, then I can't get too worked up over the relatively minor change I've proposed to the ClamAV interface.

Having said that, if the maintainers think that this particular interface change is a bigger deal than I think it is, then there are alternatives, e.g., adding a flag to enable the new exclude / include behavior and emulating the old behavior if that flag isn't specified. Or changing the names of the options for the new behavior (although I cringe a bit at the thought of that, since "--exclude" and "--include" are the most obvious and intuitive names for the options).

>I'm already leaning towards abandoning ClamAV because of instability
>in the API. Sanesecurity is the only reason I continue to use it.

It would seem, then, that the maintainers of ClamAV may share my philosophy about interface changes :-).

Jik

_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net
Re: PATCH: Enhanced exclude / include support for clamscan [ In reply to ]
Kamens, Jonathan wrote:

> On the other hand, I don't quite view clamscan as being a pipeline
> tool of the same genre as sed, tr, etc. I think of it as a
> full-fledged application, and as such, I think it's reasonable for
> this kind of functionality to be built into it if it's something that
> a lot of people are going to find useful.
>
> Other applications in the same genre with similar functionality
> include rsync, rdiff-backup, and GNU tar. All of those, like
> clamscan, could force the user to implement exclude / include
> functionality externally and then feed a file list into the
> application, and yet all of them provide complex, flexible built-in
> filtering functionality of their own.

They also typically provide a '--with-files-from=' option which might be
the right thing for clamscan too.


/Per Jessen, Zürich

_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net
Re: PATCH: Enhanced exclude / include support for clamscan [ In reply to ]
>> The regular expressions being excluded or included are compiled
>> just once and stored in memory


Brilliant!

> On the one hand, you're right that were clamscan to have a command-
> line option to tell it to read a list of files on stdin, the logic I
> built into it could be implemented externally, either with home-
> grown scripts or with a separate utility bundled with clamav. I
> certainly wouldn't be adverse to achieving my goal that way, where
> that functionality to be provided by clamscan.

I think this way would be much better than changing the interface, and
would have the added benefit of being significantly more powerful than
you probably realise. If we could supply a list of files either on
stdin or within a single text file passed on the command line with eg.
a -f flag, users and admins could incorporate ClamAV into scripts/
tasks and automated jobs a lot more easily than is currently
possible. It would also eliminate another issue I've had to work
around which is the lack of case-insensitive regex matching - I could
perform my own matching and just supply filenames within a temporary
file or via stdin.

> Other applications in the same genre with similar functionality
> include rsync, rdiff-backup, and GNU tar. All of those, like
> clamscan, could force the user to implement exclude / include
> functionality externally and then feed a file list into the
> application, and yet all of them provide complex, flexible built-in
> filtering functionality of their own.

Indeed. I don't think anyone's suggesting that the existing
functionality be removed.

> Perhaps what is needed is a compromise -- basic filtering of the
> sort I implemented, *plus* the "-f" flag (or the equivalent) for
> people who want to roll their own logic?

In my opinion, that would be an ideal solution.

> Interfaces change. Such is life. Remaining wedded to broken
> interfaces leads over time to difficult to maintain code with
> inferior functionality and performance. The changes should be
> visibly documented, and people who are using ClamAV should be smart
> enough to read the release notes when they upgrade.

The problem isn't with changing the interface, the problem is that
your solution doesn't actually change the interface, it changes the
implementation - you've just placed extra importance on the order of
the items, which wasn't there before. This could have the subtle
effect of still appearing to work, but no longer doing what the user
expected. Files which should be scanned may end up getting missed and
the user might not realise. If the interface actually changes, then
that's not such a big deal as it will simply and very obviously break
any existing procedures a user has set up, and they'll have no choice
but to change how they work.

Mark

_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net
Re: PATCH: Enhanced exclude / include support for clamscan [ In reply to ]
On Tue, 19 May 2009 15:49:23 +0100
Mark Allan <markjallan@gmail.com> wrote:

> > On the one hand, you're right that were clamscan to have a command-
> > line option to tell it to read a list of files on stdin, the logic I
> > built into it could be implemented externally, either with home-
> > grown scripts or with a separate utility bundled with clamav. I
> > certainly wouldn't be adverse to achieving my goal that way, where
> > that functionality to be provided by clamscan.
>
> I think this way would be much better than changing the interface, and
> would have the added benefit of being significantly more powerful than
> you probably realise. If we could supply a list of files either on
> stdin or within a single text file passed on the command line with eg.
> a -f flag, users and admins could incorporate ClamAV into scripts/
> tasks and automated jobs a lot more easily than is currently
> possible. It would also eliminate another issue I've had to work
> around which is the lack of case-insensitive regex matching - I could
> perform my own matching and just supply filenames within a temporary
> file or via stdin.

I added support for --file-list/-f in both clamscan and clamdscan in SVN.

Thanks,

--
oo ..... Tomasz Kojm <tkojm@clamav.net>
(\/)\......... http://www.ClamAV.net/gpg/tkojm.gpg
\..........._ 0DCA5A08407D5288279DB43454822DC8985A444B
//\ /\ Thu May 21 16:07:38 CEST 2009
_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net
Re: PATCH: Enhanced exclude / include support for clamscan [ In reply to ]
On Tue, 19 May 2009 15:49:23 +0100
Mark Allan <markjallan@gmail.com> wrote:

> It would also eliminate another issue I've had to work
> around which is the lack of case-insensitive regex matching - I could

BTW, the SVN version of ClamAV supports case-insensitive regex matching.You
just need to precede the regex with (?i), eg.

clamscan --exclude-dir="(?i)^/SyS$"

will skip /sys, /Sys, /sYs, /syS, /SYs, /SyS, /sYS, and /SYS :-)

--
oo ..... Tomasz Kojm <tkojm@clamav.net>
(\/)\......... http://www.ClamAV.net/gpg/tkojm.gpg
\..........._ 0DCA5A08407D5288279DB43454822DC8985A444B
//\ /\ Thu May 21 16:18:36 CEST 2009
_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net
Re: PATCH: Enhanced exclude / include support for clamscan [ In reply to ]
On 21 May 2009, at 3:19 pm, Tomasz Kojm wrote:
> On Tue, 19 May 2009 15:49:23 +0100
> Mark Allan <markjallan@gmail.com> wrote:
>
>> It would also eliminate another issue I've had to work
>> around which is the lack of case-insensitive regex matching - I could
>
> BTW, the SVN version of ClamAV supports case-insensitive regex
> matching.You
> just need to precede the regex with (?i), eg.
>
> clamscan --exclude-dir="(?i)^/SyS$"
>
> will skip /sys, /Sys, /sYs, /syS, /SYs, /SyS, /sYS, and /SYS :-)

Yeah I spotted that this morning. Superb news on that and the --file-
list option. Thanks :-)
_______________________________________________
http://lurker.clamav.net/list/clamav-devel.html
Please submit your patches to our Bugzilla: http://bugs.clamav.net