Hello William L. Thank you for your answer.
On Mon, May 11, 2020 at 1:01 PM William Lallemand <[email protected]> wrote: > I'm wondering if we coud have the timing directly in the logs or on the > CLI, because I'm actually also doing this kind of things, and people > could probably optimize the loading if they could see the loading time. I agree it is something which was missing while doing my tests. Remove/re-add my timer to do my tests each time I was doing some modifications; to a point where I noted to ask you if you had some tooling I missed. So that's totally something I could do in a separate patch! What is your preferred timing function? > Your patch is really interesting in my opinion, a lot of people could > benefits of something like that. Regarding the reload part, I have some > ideas to pass the ckch structures to the new processes without reloading > the files one the filesystem, but I didn't implement anything yet. ok; interesting. > I'm currently reworking the ssl_sock.c and .h files, and splitting them > in several files, so you will probably need to rebase your patch on top > of this. At the moment I separated this in 3 parts: 1 part for the > crt-list/directories, 1 part for the ckch, and 1 part for the > SSL_CTX/bind. I'm not happy yet with what I have, but I'll do my best to > push that as soon as possible in the git repository, I don't want the > 2.2 to be released in the current state of the files. good to know, and that's mostly why I avoided too much changes on the file to avoid having too much rebase work while evolving my functionality. > I don't really know the io_uring API in details so I trust you on the > implementation. Yes, I've been using it for a few months now, so I'm starting to better understand how to properly use it. There are indeed a few different options where you could easily get trapped, and making the code more complicated. I however was planning to ask Jens for a quick review when I will have something more feature complete. > Regarding the name of the file, ssl_load.c, it is really generic, I > don't mind having a more specific file only for loading the SSL > certificates with io_uring. Yes I totally agree; as it was more of an experimentation I will do a second path on the naming and improve that. As I also started some poll changes, it also helps me to better name things to make elements more reusable. > I'm wondering if we couldn't push directly the certificate in an X509 > struture instead of storing it in a intermediate buffer, and use the > ckch instead of a specific cert_iobuf. That just some thoughts, I don't > know io_uring enough. I will look into that and it was also on my todo but for now I considered it ok for a first implementation. It mostly depends if we are able to properly allocate memory at the right time, while keeping the code clear enough. For now I considered it as a possible incremental improvement. Thanks for the input. > The crtlist files could also benefits from this, since they can contain > a lot of certificates too. Yes, indeed, for now I limited myself to the cases I was concerned about to avoid too much testing. I will improve that. I should have warned about it in my commit message. > I'm seeing that you are using in ssl_load_preparetree() the > SSL_SOCK_KEYTYPES_NAME array, which is only used for merging a > certificate bundle. I know that this can be confusing in the code > because the bundle implementation is crappy. But in a directory we > don't use any filename extension to load a PEM, we load everything as a > PEM file with the exception of .key, .ocsp, .sctl and .issuer, which are > loaded separatly with their own functions. (The cert_exts[] array is > used for that in ssl_sock.c) ok, good to recall all files should be taken into account; for now I blindly implemented my only known case. This will be part of the above point to take into account all possible options. > I'm going to remove the support for multi-cert bundle once 2.2 is > released, so it will simplify a lot of things. I encourage people > writing new features to not support multi-cert bundles, more > particularly on the CLI. So you mean that example.pem.rsa example.pem.ecdsa will be loaded separately as all the other certificates? I'm not 100% sure what you meant behind removing support of "multi-cert bundle". > Unfortunately your patch is too late for 2.2, but I think it could be > great for the next release! Yes, and it was clear for me since the beginning, I would not have the time to finish things early enough. Thanks a lot for your feedbacks. I will try to combine the one from Willy and yours for a future submission. I also hope we could find a good middle point between something perfect and something we could improve along the time. -- William
