Re: [mmh] [PATCH 9/9] Factor out draft file creation from replout()

[markus schnalke]

[2016-09-09 18:58] Dmitry Bogatov 
>
> When I started working on patch, which would extract file creation
> with `m_gmprot', I asked myself more general question -- what is the
> point of `m_gmprot'?
> 
>   $ man mh-profile
>  Msg-Protect:0644
>   An octal number which defines the permission bits for new 
> message
>   files.  See chmod(1) for an explanation of the octal number.
>   (profile, default: 0600)
> 
> In code, it is passed to umask(2) anyway. So, as I understand matter,
> Msg-Protect is just a way to specify mmh-local umask.

Yes.

> If my understanding is correct, we can eliminate it fully. If not, please,
> I need clarification.

As I couldn't come up with a clear oppinion about it, back then,
I left it as it was. Haven't made much fuzz about it, but at the
same time I haven't seen much worth in it, equally.

I always thought that it existed because mail could be regarded
as needing more privacy concern, thus the files should be stronger
protected (the default is 0600, that equals a umask of 077) in
contrast to normal umasks, which are often 02 only.

For the mail storage we can solve the privacy problem by setting
the permissions for ~/Mail appropriately, but mhstore(1) seems to
be the relevant point. If you set Nmh-Storage in the profile to
store all files of to a central directory, which all users use,
then Msg-Protect is relevant.

I would like to have Msg-Protect removed, but I am not sure if we
cut off valuable possibilities at the same time. Actually, the
point is that my feeling is that I don't see far enough in this
topic to be conviced of any decision.

But I would say that we can remove it from everywhere except
from mhstore(1), without problem.


More oppinions or ideas on this?


meillo

Re: [mmh] [PATCH 9/9] Factor out draft file creation from replout()

[markus schnalke]

[2016-09-08 15:33] Dmitry Bogatov 
> 
> I would move to sbr/ after I locate another call site.

I suggest that this could be done in one go: try to rework all or
most of the places we do the similar thing.


> About lookup --
> you look at function name and understand that file is created and
> FILE* is returned. Since it is assigned to `out' variable, it is
> created for write. No need to lookup actual code.

To explain my view of the situation:

You assume that abstractions would be perfect. I believe that
in reality abstractions are leaky, hence I don't read over the
function call but do the lookup ... because you never know! I
do want to understand code right down to the bottom. Maybe I
look as being arcane in this respect ... I can assure you, I'm
not. :-)

If there is exactly one call of such a small function, then the
lookup overhead is large. If however the function is used in
many places, then the overhead decreases as I have to do the
lookup only once for all of them.


> And, it is much easier to locate code duplication when you have many
> "several lines" functions, than when you have many code in every
> function.

Of course. I would appreciate to have such a function outfactored.
I do only suggest to first examine the other places that have
similar code and try to convert them in one go. Because only if
you know about the other places, you know if the shape of function
you created from this one places fits for all the others as well.

To sum it up again: I think that this patch contains a worthwhile
idea, but I don't think it is finished already. Take the idea
further and then create a patch that solves the issue more
completely.


meillo

Re: [mmh] [PATCH 9/9] Factor out draft file creation from replout()

[Dmitry Bogatov]

[2016-09-08 13:58] markus schnalke 
>
> part   text/plain1630
> Should this be a generic function (included into sbr/)?
> There probably are other places where we create files
> with reduced permissions. This patch here does not fully
> convince me -- it pays the encapsulation of a few lines
> with an additional lookup --, but it might be a starting
> point for a larger improvement. Then the deal could be
> much better.

I would move to sbr/ after I locate another call site. About lookup --
you look at function name and understand that file is created and
FILE* is returned. Since it is assigned to `out' variable, it is
created for write. No need to lookup actual code.

And, it is much easier to locate code duplication when you have many
"several lines" functions, than when you have many code in every
function.

Probably, following quotes from Linux coding style (doc/CodingStyle)
can be convincing:

GLOBAL variables (to be used only if you _really_ need them) need to
have descriptive names, as do global functions.  If you have a function
that counts the number of active users, you should call that
"count_active_users()" or similar, you should _not_ call it "cntusr()".

Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well.

Another measure of the function is the number of local variables.  They
shouldn't exceed 5-10, or you're doing something wrong. [...]

Re: [mmh] [PATCH 9/9] Factor out draft file creation from replout()

[markus schnalke]

Should this be a generic function (included into sbr/)?
There probably are other places where we create files
with reduced permissions. This patch here does not fully
convince me -- it pays the encapsulation of a few lines
with an additional lookup --, but it might be a starting
point for a larger improvement. Then the deal could be
much better.


meillo


[2016-09-08 13:04] kact...@gnu.org
>
> From: Dmitry Bogatov 
> 
> ---
>  uip/repl.c | 22 +-
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/uip/repl.c b/uip/repl.c
> index 8a40543..9e39b2a 100644
> --- a/uip/repl.c
> +++ b/uip/repl.c
> @@ -370,8 +370,18 @@ docc(char *cp, int ccflag)
>   }
>  }
>  
> +static FILE*
> +fcreate_with_gmprot(char *filepath)
> +{
> + int mask = umask(~m_gmprot());
> + FILE *out = fopen(filepath, "w");
>  
> -
> + if (!out) {
> + adios(EX_CANTCREAT, filepath, "unable to create");
> + }
> + umask(mask);
> + return out;
> +}
>  
>  static void
>  replout(FILE *inb, char *drft, struct msgs *mp,
> @@ -382,16 +392,10 @@ replout(FILE *inb, char *drft, struct msgs *mp,
>   int i;
>   struct comp *cptr;
>   char **ap;
> - int char_read = 0, format_len, mask;
> + int char_read = 0, format_len;
>   char *scanl;
>   unsigned char *cp;
> - FILE *out;
> -
> - mask = umask(~m_gmprot());
> - if ((out = fopen(drft, "w")) == NULL)
> - adios(EX_CANTCREAT, drft, "unable to create");
> -
> - umask(mask);
> + FILE *out = fcreate_with_gmprot(drft);
>  
>   /* get new format string */
>   cp = new_fs(form, NULL);
> -- 
> I may be not subscribed. Please, keep me in carbon copy.
> 
> 
>