Request for review of Samba VirusFilter preparation for merging with Samba

Simo simo at samba.org
Sun Sep 11 04:44:03 UTC 2016


On Sat, 2016-09-10 at 14:02 -0600, Trever L. Adams wrote:
> On 09/10/2016 12:54 PM, Jeremy Allison wrote:
> > 
> > On Sat, Sep 10, 2016 at 12:00:26PM -0600, Trever L. Adams wrote:
> > > 
> > > > 
> > > > Then -- no lines beyond 80 chars please.
> > > Is this hard and fast? There are a few things that are right
> > > around
> > > 80-83 characters that would be difficult to split up.
> > Yeah, it is really. Just break the lines up.
>                 scan_errno = virusfilter_h-
> >infected_file_errno_on_open;
> Is of concern to me. It seems like years ago I found that some
> compilers
> would not compile a line like above if it was broken at -> and that
> is
> the only way I see to shorten the line.
> 
> Any suggestions? Other than that I believe I have not made all of the
> lines <= 80 characters, even in header files.

2 ways:

1. use a short name for the member

2. 		scan_errno =
		   virusfilter_h->infected_file_errno_on_open;

I prefer shorter variable names personally, in cases like this.

HTH,
Simo.


			
> > > Does this apply to function definitions in .h files?
> > > > 
> > > > The virusfilter_io abstraction could see a lot of code re-use
> > > > from our
> > > > base libraries. We have for example write_data_iov that you
> > > > could
> > > > re-use. If that does not do what you need, can we share this
> > > > code
> > > > somehow?
> > > I will look into that and work with the author/maintainer.
> > > > 
> > > > 
> > > > If I get the code right, you're not linking to proprietary
> > > > libaries
> > > > but connect via sockets to an AV daemon. Is that right? We
> > > > never
> > > > really approached AV because that usually means linking to
> > > > proprietary
> > > > libraries. This problem is gone now?
> > > > 
> > > > Volker
> > > > 
> > > I only worked with the clamav part, but I quickly looked over the
> > > others, as far as I know there is no linking, only connecting via
> > > sockets. I believe there was a planned module to do external
> > > calls
> > > (fork/exec/std(in|out) redirection), but I don't believe that has
> > > been
> > > done, just planned.
> > > 
> > > Other than write_data_iov and other reuse issues, I believe I
> > > have done
> > > as you have asked.
> > Looking at vfs_virusfilter_vfs.c you are now mixing talloc with
> > SAFE_FREE.
> > That will not end well :-).
> Thank you. I had the inverse when I first adapted the function from
> vfs_recycle. Thank you for keeping me honest about it.
> > 
> > 
> > Also there need to be NULL return checks on all uses of talloc.
> > They are missing in vfs_virusfilter_clamav.c for example.
> I just did my best to check all of these. They were really easy to
> handle. Thank you for catching these as I haven't done much in the
> scanner specific files until now.
> > 
> > 
> > Hope you don't think I'm being too picky. This is *really*
> > great work and I'm very happy you're doing it. I'd certainly
> > like to get this in a state where we can merge and support
> > it formally.
> > 
> > Cheers,
> > 
> > 	Jeremy.
> > 
> Not at all! Thank you for the feed back. I hopefully have caught all
> of
> your suggestions.
> 
> Trever
> 
> 
> 




More information about the samba-technical mailing list