Re: [Nbd] [PATCHv5 1/7] Add GnuTLS infrastructure

[Alex Bligh <alex@...872...>]

Wouter,

>> I said that I didn't want to introduce a dependency on pkg-config when none
>> currently existed, and I don't think I heard back.
> 
> Ah, that's possible :-)
> 
> I think that's not something you should be worried about. pkg-config is
> pretty universal these days; also, it makes it (must) easier on the user
> to point to an alternative implementation of particular things.

I had to specifically get it for OS-X recently and was surprised nothing
else had needed it, but OK.

> 
>> But no great shakes.
>> 
>> There's a simple answer to most of the stuff below:
>> 
>> The code is not nbd code. It's code from:
>>  https://github.com/abligh/tlsproxy
>> which is an MIT licensed TLS proxy written as an example for
>> GnuTLS (not specifically for nbd).
> 
> I know that, but you're suggesting it be merged into the nbd repository.
> Once you do that, it becomes nbd code, and we'd rightfully be expected
> to maintain it here as well.

Well that's fine, but it's (surely) easier to maintain it just by
copying over the version from upstream!

> 
>> It's a byte-for-byte copy of the files, and I don't particularly fancy
>> maintaining two code bases.
> 
> Then don't, but essentially you're saying I can now *also* not make
> changes to these files once it's merged into the nbd repository. Sorry,
> but that's not going to fly. If at some point I think nbd needs to do
> something different and it requires changes to some of the files that
> are part of tlsproxy and are in the nbd repository, I'm not going to
> hold back just because it's "external" code.

No I'm not saying other people can't change it (hey, I MIT
licensed it specifically so they can do what they like with it).
I'm saying the easiest way of maintaining it is not to change it.

> If you make it a library (on which nbd is then going to depend) then
> okay, we could do that. I don't think this code is particularly useful
> as a library, but it's your call.

Neither do I.

> At any rate, I do not want to limit the nbd TLS implementation to the
> capabilities of some random unrelated code that just happens to
> implement something close to what we want. Your tlsproxy is useful, and
> yes, it makes sense to use it for the client, but I am absolutely not
> convinced it's the right (long-term) approach to TLS in the server.

In the server I agree 100%. It's just 'good enough for now'.
In nbd-tester-client.c and in the client I think it's a pretty good
approach.

I've no great attachment to keeping it exactly the way it is, but
I don't really see that renaming things etc. is going to help
maintenance.

I can fix the indentation by running lindent on it if you like
(so it's consistent in the project). At least that means updating
it (until there is a changed introduced for good reason) is a matter
of cp & lindent.

> 
>>> [...]
>>>> +  if (socksetnonblock (cryptfd, 0) < 0)
>>>> +    {
>>>> +      errout (s, "Could not turn on blocking: %m");
>>>> +      goto error;
>>>> +    }
>>> 
>>> Why not reuse the function for the same purpose in nbd-server.c? We can
>>> obviously move it elsewhere if needs be, but it seems wrong to implement
>>> the same thing twice in one codebase.
>> 
>> Because I neither want to expose that from tlsproxy, nor (for obvious
>> reasons) have tlsproxy call nbd.
> 
> Then make something common that both use? This is the sort of minor
> useful stuff that would be well served by a file containing some utility
> functions.

This all boils down to making it easy to maintain (or not).

>> Indentation I think you will find is totally right, because it
>> was run through GNU's 'indent' program. Unless something really odd
>> has happened!
> 
> Point being, the if has six spaces, the break has a tab. The result is
> that (in this mail) the break indends less than the if. In your reply,
> it probably won't anymore.

I've just run gnu indent on it (which made one minor unrelated change)
to check this.

Here's the output of hexdump:

00002aa0  21 71 75 69 74 20 28 73  29 29 3b 0a 20 20 20 20  |!quit (s));.    |
00002ab0  20 20 69 66 20 28 71 75  69 74 20 28 73 29 29 0a  |  if (quit (s)).|
00002ac0  09 62 72 65 61 6b 3b 0a  0a 20 20 20 20 20 20 69  |.break;..      i|

I believe that is indented correctly according to GNU standards.

"if (quit (s))" is indented by 6, and "break" is indented by 8. The GNU
indent standard is that tabs are 8, and are used as much as possible,
and spaces used elsewhere. Therefore the "if" is indented with 6 spaces,
and the "break" by one tab. Replacing the tab by 8 spaces and rerunning
indent again turns it back into a tab.

So the issue that the GNU coding standard requires use of hard tabs
and these don't work well when transmitted through mail (particularly
in patches). The actual formatting is 'correct' (in the sense of
conforming to what 'indent' produces).

Anyway, if I run lindent instead presumably this issue will go away.

-- 
Alex Bligh