Re: [hackers] [slock] [PATCHSET] Some improvements and more security

From: Quentin Rameau <quinq_AT_fifth.space>
Date: Mon, 12 Sep 2016 13:13:39 +0200

> > First patch:
> > - I agree with the structuring of the xrandr part, good!
> > - But not with the localization of every data, they're used across
> > the whole program and you're just over-complicating functions here
> > imho. The program is simple and clear enough not to have to get
> > triple pointers passed as function parameters.
>
> the triple pointer is just used once in the cleanup function, as after
> freeing the locks we set the locks double pointer to NULL.
That was just one example of the rest of the complexity it brings in
here.

> > - un-typedef, that's not an issue here, why changing it besides
> > satisfying your personal taste?
>
> because it would be inconsistent with the xrandr-struct.
It would be inconsistent with… An inconsistency you're bringing in? ^^
> There is also
> no reason to typedef struct anyway and just hides the underlying
> machinery for no good reason.
As I said, personal taste.

> > Second patch:
> > - fine by me, renaming variables patch ;p
>
> You wouldn't believe how much difference it makes to improve variable
> naming.
No I wouldn't! I have no clue!

> > Third patch:
> > - the rval renaming belongs to the second patch
>
> It's debatable if you think of the second patch as a renaming patch,
Is it not? Does it make more sense to have it in a patch which changes
where we get the user login from?
> but in the end I don't see it as a big issue.
Not really indeed (either way).

> > - I agree with the removal of the $USER, we discussed it before this
> > patch on IRC. But maybe we should have the reasoning behind that
> > from the original author who put it here.
>
> The reasoning is that we don't want a bloody seduid program depending
> so strongly on environment variables. For some things, it's sufficient
> to just kick them out because they are so obviously wrong.
Not *that* reasoning, as I already said, I already told you I was for
getting rid of it.
I mean the reasoning of why it was introduced in the first place, to be
sure something is not missed here.

> What has been bugging me for quite a while is this DPMS comment that
> was added there for no reason.
[…]
Get rid of it.
Received on Mon Sep 12 2016 - 13:13:39 CEST