Request for review of Samba VirusFilter preparation for merging with Samba

[Trever L. Adams]

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.
>
>> 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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 872 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160910/c2426273/signature.sig>