[PATCH] Samba VirusFilter (version 11)

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Samba VirusFilter (version 11)

Samba - samba-technical mailing list
I believe this contains all requested changes. If I have missed
anything, please let me know.

SATOH Fumiyasu, I am sorry, I accidentally have left you out of the
recent emails. I hope this still meets your approval.

Thank you everyone for helping get this here.

Trever

P.S. git am should work with this patch and should result in 5 commits.


samba-virusfilter.patch (143K) Download Attachment
signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 11)

Samba - samba-technical mailing list
Trever
Status of functions returning virusfilter_result should be checked
against correct constants (not true or false).
A few more nits.
Jim

Tests for type virusfilter_result should be against correct constants -
not against true.
Or, it should be assigned to a bool variable like virusfilter_io_readl.

result != true should be result != VIRUSFILTER_RESULT_OK (many places)

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + become_root();
> + result = virusfilter_io_connect_path(io_h, config->socket_path);
> + unbecome_root();
> +
> + if (result != true) {

Should be 'if (config == NULL) {'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + config = talloc_zero(handle, struct virusfilter_config);
> + if (!config) {
> + DBG_ERR("talloc_zero failed\n");

Missing space after 'if'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + if(tmp != config->quarantine_dir) {
> + TALLOC_FREE(tmp);

Variables should be tested against 0 - 'if (... > 0) {'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + test_prefix = strlen(config->rename_prefix);
> + test_suffix = strlen(config->rename_suffix);
> + if (test_prefix) {
> + rename_trap_count++;
> + }
> + if (test_suffix) {
> + rename_trap_count++;
> + }

Multiple places for text_prefix and test_suffix in this block should be
'... > 0'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + if (test_prefix || test_suffix) {
> + ok1 = parent_dirname(mem_ctx, fname, &dir_name, &base_name);

Should be 'if (iov_p->iov_base == NULL) {'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + iov_p->iov_base = va_arg(ap, void *);
> + if (!iov_p->iov_base) {
> + break;

result is bool
Use 'if (!result) {'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + if (result != true) {
> + return result;
> + }

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + result = virusfilter_io_readl(talloc_tos(), io_h, read_line);
> + if (result != true) {
> + DBG_ERR("virusfilter_io_readl not OK: %d\n", result);
> + return false;
> + }

Multiple places in two virus scanners should be 'reply_token != NULL' in
if tests

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + reply_token = strtok_r(NULL, " ", &reply_saveptr);
> + if (reply_token) {

Should be 'if (reply_token == NULL) {'

On 12/28/2017 2:27 AM, Trever L. Adams via samba-technical wrote:
> + reply_token = strrchr(reply, ' ');
> +
> + if (!reply_token) {
> + DBG_ERR("clamd: zSCAN: Invalid reply: %s\n",
> + reply);


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 11)

Samba - samba-technical mailing list
On 12/28/2017 08:41 AM, jim via samba-technical wrote:

> Trever
> Status of functions returning virusfilter_result should be checked
> against correct constants (not true or false).
> A few more nits.
> Jim
>
> Tests for type virusfilter_result should be against correct constants
> - not against true.
> Or, it should be assigned to a bool variable like virusfilter_io_readl.
>
> result != true should be result != VIRUSFILTER_RESULT_OK (many places)
Jim

Thank you so much for your feed back. I am sorry for the delayed
response. I missed this email last week. Your feedback made me realize
that the consistency I once hoped for (using the virusfilter_result)
everywhere was not a good idea. I have tried to 'simplify' the return
types in addition to making the changes you suggested. If bool is
enough, I am now using bool. If it needs an int, use that, if it needs
the full virusfilter_result, use that.

As I keep missing things, I am attaching the incremental patch.
Hopefully, this will help find the last nits, coding style violations
and any bugs (which this process has helped me uncover and fix) remaining.

Thank you to all who have been so helpful.
Trever

0006-Do-more-to-correct-coding-style-and-nits-as-Jim-put-.patch (13K) Download Attachment
signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 11)

Samba - samba-technical mailing list
On 1/1/2018 2:45 PM, Trever L. Adams wrote:

> On 12/28/2017 08:41 AM, jim via samba-technical wrote:
>> Status of functions returning virusfilter_result should be checked
>> against correct constants (not true or false).
>> A few more nits.
>>
>> Tests for type virusfilter_result should be against correct constants
>> - not against true.
>> Or, it should be assigned to a bool variable like virusfilter_io_readl.
>>
>> result != true should be result != VIRUSFILTER_RESULT_OK (many places)
> Thank you so much for your feed back. I am sorry for the delayed
> response. I missed this email last week. Your feedback made me realize
> that the consistency I once hoped for (using the virusfilter_result)
> everywhere was not a good idea. I have tried to 'simplify' the return
> types in addition to making the changes you suggested. If bool is
> enough, I am now using bool. If it needs an int, use that, if it needs
> the full virusfilter_result, use that.
>
> As I keep missing things, I am attaching the incremental patch.
> Hopefully, this will help find the last nits, coding style violations
> and any bugs (which this process has helped me uncover and fix) remaining.
>
> Thank you to all who have been so helpful.

Trever

These incremental changes address all of my concerns.

Jim

Could 'result' be renamed 'ok' to match the other usage patterns (i.e.
enhance readability)?

On 1/1/2018 2:45 PM, Trever L. Adams wrote:

> @@ -677,13 +676,13 @@ bool virusfilter_io_writefl_readl(
>   result = virusfilter_io_vwritefl(io_h, fmt, ap);
>   va_end(ap);
>  
> - if (result != true) {
> + if (!result) {
>   return result;
>   }
>   }
>  
>   result = virusfilter_io_readl(talloc_tos(), io_h, read_line);
> - if (result != true) {
> + if (!result) {