[Citadel Development] Re: New results from Uncensored import -- concerns about visit records
Glad you figured it out. The builds are tricky. I don't think we could do much with the room gen number even before the new Visit. The room gen is between the room number and the user id in the database index, so we are stuck unless we convert the entire database. In my first go round with Visit when you found it took so long to convert the database, I had moved the user id to the beginning of the index, so we could load just based on user and speed up purging a user's Visit records.
[Citadel Development] Re: Question about valid login user names
The email makes sense. I saw that code, but wondered if alias needed to be supported. The attacks are a pain, but I have a pretty good custom solution in place. The Citadel blocking and firewall blocking just did not work for me. I use a virtual machine and the attacks eventually mess up the network interface and the attackers rotate through thousands of IP addresses which my firewall just could not handle. I now reset the network interface periodically and drop socket connections as soon as Citadel opens the socket and sees that the IP is supposed to be blocked. The biggest improvement you made was updating the database. Before that, and working out a better blocking algorithm, Citadel crash and the database was corupted and I was having to reload from backups. Ugh.
[Citadel Development] Question about valid login user names
My server has been under constant attack by brute force user/password login attempts. I have my own blocking in place, but in the attacks, I noticed that Citadel allows users to log in with their user name or an alias for the user. Should aliases be allowed for logging in?
[Citadel Development] Re: New results from Uncensored import -- concerns about visit records
Can you give me a sample Visit dump record? Should look something like this: visit|1|1728342664|2|0|4|0|*:0| The only thing I can think of that might be an issue in the dump/load would be the third field (room gen). If the gen number is big enough, it might be the root cause. If you search the dump file, you should find a room with the matching number, so if it does not exist, the visit value is the issue. If that does not show what is wrong, I will create a 32-bit VM and try to debug it. > Fri Oct 25 2024 23:42:23 EDT from IGnatius T Foobar Subject: New >results from Uncensored import -- concerns about visit records > > >As I might have mentioned before, Uncensored is still running on 32-bit >Linux and I've been procrastinating for a long time over getting it >converted to 64-bit, partially because until this year it was running >on really crappy hardware and the dump/load would take forever. This >week I finally did a test run, exporting from a snapshot of the >production system and importing on the target 64-bit container. Here >are some results: > >* The dump took about 8 minutes during a period of normal load and >consists of 776153 rows consuming 15 GB. >* Loading that dump onto a new environment took about 23 minutes and >loaded all but 4 rows. All of the bad rows were user records, and >they're probably junk anyway. > >* And now the issue: it appears that the visit records did not import >properly at all. When I logged in, all messages were new in all rooms. >I also noticed that mailboxes were appearing with the forum view >instead of the mailbox view, also indicating a visit problem. > >* The dumped visit records appear to be in the correct format, at least >at first glance. I'll check on this some more over the next couple of >days. > >More info to follow if I can find it. > > > >
[Citadel Development] Re: Berkley backend logic
I only got into the backend code because I wanted to see the timing of the IMAP and SMTP code without the database accesses. I am impressed by the current code, because the thread routines are really very good. I go back to the days of when even function calls were not great for performance and when I saw the number of times that the function was called and it was in the top timing, I took a look at it. I tried to change the code to check the thread id and only get the thread data when it changed, but that took longer than the existing code. The only improvement I could make was to only get the data once a function which assumes the thread does not change during the processing of the function. I did it by adding a macro call at the beginning of each function that used TSD which adds a local variable and then changed the TSD macro to have all the calls use it, so minimal code change. I can flip back and forth with the original code by just changing the macro definitions so it was easy to try some different implementations. For changing code to support things, I tend to use macros like Citadel has for CC and TSD. CC crosses threads enough that if it is a little less efficient to keep it thread safe, it makes up for it in not making cross thread mistakes in all the supporting code which is more important. I also like to use macros for implementing code outside of the repository, like my profiler, memory checker and IP blocker to stop DOS and brute force password attempts. It helps when I merge with the repository to minimize the complexity of the merge so I can upgrade without reimplementing. Macros can be very powerful in testing and not affect production code. If anyone is looking at memory in the future, it may be good to go back and free up the memory at shutdown for testing, but compile it out on production.
[Citadel Development] Berkley backend logic
Good to know about performance and memory. While I was checking performance of the IMAP and SMTP, I noticed that the backend database code calls bdb_get_tsd() for every TSD reference and gets the thread specific structure each time. Does the Berkley database ever switch threads when calling the Berkley API? I played with it and did not see it switch threads in any of my testing. If we assume that it does not switch threads, and only call bdb_get_tsd() once a function call, I showed about an 8% improvement in performance in receiving test emails.
[Citadel Development] Re: Hardcode Visit legacy record sizes so will not change if defines change
It wasn't dumb :-) I just thought Out of the Box/Struct. It just wasn't scalable, so I had to come up with something to add all the new ones. On the email address ideas, I looked at changing the email address buffering to their own defines, but it really was messy. Probably just better to see if we want to change the limitation and reimplement it then. If it ain't broke, don't mess with it. You had mentioned something about performance a while back. What specifically were you trying to improve? I have my own home grown profiling and memory checking modules that I have implemented on Citadel in my personal branch. I found the GNU profiler useless because it is sample based and does not include I/O. Mine requires adding a macro to the beginning of each function and a macro for each return, for the functions I want to report on, but it gives me accurate info with a little overhead in testing and compiles out for production. If you have an area you are interested in, I can take a look. My memory checker does not require an source changes, just including a header and module in the library. It macros the malloc(), etc into my library and then I can do anything I want. Currently I track all the allocations and frees and report whatever it left over at shutdown. It only is good in testing since it does add a good amount of overhead checking. I have not found any leaks, overruns or underruns in the code yet :-)
[Citadel Development] Re: Some recommendations and questions
Was not able to reproduce the coredump, but the only thing that it could have been was a bad memory copy. The BODY partial parsing and computation did not error check the syntax or if the start of the partial was past the end of the string the math was bad for unsigned variables. Bad syntax would probably do some very unpredictable things and if the start was past the end, the unsigned logic would make the length huge. Fixed both. I am going to work on some tests to try to automate my testing so I can test some of the bigger changes. I will also check out some of the clients you mentioned.
[Citadel Development] Re: Some recommendations and questions
Interesting history. I kind of wondered because most people do not follow the old standard when writing C and use some of the C++ features. I like the old. I think I will go ahead and set a define for the max email address size, and update the code to use that as the limit, but not actually increase the current limits. Later we can increase it and test with different mail clients. I know that when I tested against Outlook with a ridiculously sized email address, it had problems displaying it, but did not harm anything. What clients do you test against? I will do this after I fix a bug in the IMAP code. I update my server to the latest and everything looked great and within a day it core dumped. Turns out it had nothing to do with any of the recent changes. It was very old code processing the BODY statement partial specification. I was not able to reproduce it, but looking at the code, if someone passed some bad start/length, it does some bad math with unsigned integers which is probably what caused it to try to copy a bunch of characters from an empty string and dumped. I am looking forward to the new WebCit.
[Citadel Development] Some recommendations and questions
I wanted to bounce a couple of things off of you to see if there are any down sides my implementing any of these. Systemd Changes === Recommend changing citadel service configuration "After" to use "network-online.target" instead of "network.target". "network.target" does not wait for the network to actually be operational and I have run into race conditions on startup. Recommend changing webcit service configuration "After" to use "citadel.service" instead of "citadel.target". Limits on Email Addresses The code is not consistent on what size the email addresses can be in message. Early on, I ran into Citadel core dumping on iCloud addresses. It doe not seem to create such long ones now, but a year or two ago, they would create addresses even longer than 256 characters (the standard recommendation). The standard also says that allowing longer ones is up to the server, but may be a goo idea. I had changed the code to accept very long addresses, and check more, but not to truncate/reject addresses or recipient lists. There was a comment in the code when truncating the recipient list to about 1000 characters that it was so that the library function would not choke on it. When I increased the buffer sizes, and allowed very long addresses, I did not seem to have any issues. Is there an issue with the libraries anymore? I found that Outlook does have some display issues with very long addresses, but my thought is if the user has long addresses in email, it is up to the client to handle them, not the server. Recommendations: - Create a new define specifically for the max length of an email address and recipient list. Even if we decide to limit an address to 256 and recipient list to 1K, the code should use a define to check and limit. Either remove the limits or make the defined limits very large. The buffers dealing with the addresses and list are mostly just local variables, so only stack memory is used so no real memory overhead like malloc(), so just setting a large limit and ensuring all the code validates it should be enough.
[Citadel Development] Re: Fix Visit IMAP not to return error on empty flags specification
Your changes are fine. I had the hardest time originally when I was trying to develop the flagging stuff with all the changes going on. The changes required a lot of updates to make performance optimal and constantly integrating the changes was very time consuming as to not break the code. Git merge is doing a good job now of integrating the changes. As for returning structures, I too was programming back in the late 70's, but did not really start heavily into C until the mid-80's, so a lot of the limitations were changed. I still like C because it is efficient and flexible. It is very powerful for good programmers and makes bad programmers worse :-) I rarely return structures because it requires copying all the data on return, but in this case, it was cleaner than all the pointer parameters and easier to expand later. I normally just have the caller pass a pointer to a structure, but the structure was so small, I had fun. I had noticed that a lot of the calls just wanted to switch rooms and did not use the metrics, so I just went and changed all the calls. When you want to support IMAP4rev2, let me know. It is going to require some work to integrate and test.
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
Thanks for catching that. You were right that I was thinking IMAP new messages when I was updating the goto code. I forgot the original code was based on Seen.
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
It worked. Excellent. The code is committed. If you find any issues, let me know.
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
That worked a lot better :-) I should have though of that. I was so www based, I was trying everything relative to that. ssh is actual filesystem path. Duh. Now the the next hiccup. Tried to push and get errors that I cannot write to ./objects
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
When pushing, I keep getting 403 errors from the URL https://code.citadel.org/citadel.git I have tried adding the user and password to the URL but get the same thing. I also tried to switch to ssh, but I cannot seem to get the repository path right in the URL and it always returns an error. :citadel.git or :/citadel.git or :citadel/citadel.git or anything else I have tried always returns "does not appear to be a git repository".
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
I am ready to push my commit to the repository. Problem is I cannot figure out the right origin to use or I am just messing up the command (I really do not use git much :-P ) What arguments do I use for git push to push my cloned master to the repository? > Mon Sep 09 2024 14:01:59 EDT from IGnatius T Foobar Subject: Re: Back >after a long time. Want to add the support for IMAP flags > > Ok, your login should be ready to go. I am out of town this week (in >fact, I'm writing this from an airport) so there is little chance of me >writing conflicting changes. Actually I might get bored and start >coding ... but I'll stick to webcit-ng :) > > > >
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
The only thing I need to do is remove the support for the old version in ctdlload. I still have to check the field count (v1001 has fixed 9 fields which newer versions will never have) so it will just return failure rather than converting a couple of the fields. I actually did my implementation against v1001 so I had a stable version to test against. I pulled master a few days ago and ported the code there and that is what I am testing. I updated master last night so my code has all of your cleanup. I will send you my login info so I can check the code in. > Sun Sep 08 2024 18:40:01 EDT from IGnatius T Foobar Subject: Re: Back >after a long time. Want to add the support for IMAP flags > > >>I implemented everything on the v1001 tag. I have pulled the latest in >> > > >>git and am creating a local branch with my updates. Should I commit >>the code to master after I finish testing? > >I have this annoying habit of going through code and doing syntax and >style cleanup when I am attending boring meetings at work but can't >commit to focusing on real development. The result is that if you have >a patch it might not apply cleanly because it might be against code >where I changed "return(1);" to "return 1;" or something silly like >that. > >I'll stop doing that now that I know you have stuff coming in. Please >sync up with git master and then if everything is working then yes, it >would be easiest if you could just commit to the main line. Please PM >me with your desired username and password because we're not using >GitLab-CE anymore -- it was just consuming too many resources so I >switched back to regular `git` with the `cgit` web front end. So when >we create your account it'll just be an ordinary account on the server >with permission to write to the repository. > >It sounds like we're in agreement on what it has to do: >* The server must be able to read records in old or new format >* The server will always write in the new format >* ctdldump must be able to read in old or new format, but always dumps >in new format >* ctdlload understands the new format only > >I like what you're doing. The purpose of the 'visit' record is to >identify the relationship between a user and a room, and with these >extensions it will more clearly identify the relationship between a >user and messages within the room. As the Citadel System evolves, the >concept of a "room" will begin to fade for users on the web UI, but it >will still be the primary internal representation. So the more >flexibility we have, the more useful it will be. > > > >
[Citadel Development] Re: Back after a long time. Want to add the support for IMAP flags
When I reimplemented, I did 1 and 2. I also changed ctdldump and ctdlload and they seem to work well. ctdldump writes the new format for all records. I was not sure about supporting old versions, so I did. I made ctdlload support the old and new formats. The new format always has more fields than the previous format (fixed) because of the added flags, so it was easy to support. I did a few things to improve View performance overall and as the records are saved in the new format, we should see some reduction in the database size. I implemented everything on the v1001 tag. I have pulled the latest in git and am creating a local branch with my updates. Should I commit the code to master after I finish testing?
[Citadel Development] Back after a long time. Want to add the support for IMAP flags
I was swallowed up by life for a long time. I am back now and want to help develop in Citadel. Last time, I created a version to support the addition of the Flagged, Draft, Recent and Deleted flags and also reworked the Visit support to allow any size message sets while reducing the record size dramatically. Problem was when you tested it, converting all the Visit records during upgrade took forever. I have rewritten it to convert the records on the fly to the new version and have designed the structure to be expanded and changed much more easily. I removed the support for Recent. Did a lot of research on it before and it was not implemented properly and nobody really uses it. It is not a flag at the message level, but at the room level. Has to be maintained in it's own database record. Since IMAP4rev2 has deprecated the flag, and it is a lot of work to maintain, it is not worth the effort. IMAP4rev2 says you can output RECENT to stay compatible to IMAP4rrev1, just assume zero recent, so that is what I implemented. I can create a patch and submit for review/testing if that is the best way to do this. I don't have large databases to test against or test suites to cover everything. Let me know how you want me to submit it.
[Citadel Development] Re: IMAP Flags branch?
Sorry. I have been sidetracked for about a month with work and life. Going from upgrading the database to supporting multiple versions of the Visit records required some thought and will require rework of a lot of the implementation. I had optimized a lot assuming the database was at one Visit version, so handling different versions requires changes to the logic and functions calls to keep it optimized. I also re-thank the implementation of the Recent flag. I ended up removing the branch since I needed to redo a lot and there were a lot of changes to the repository, so merging was getting out of hand. I will be having a little more time now, so I will be going back to implement the flags.
[Citadel Development] y creashRe: Database Recovery Tools
My crash was on a dev system. Since the DB is not in master, I just use the tar from EasyInstall and build it. When it crashed, I tried to run the db_ tools and they give me the error about not having included the recovery part. I did a clean EasyInstall and got the same error. I played around with the build of Berkley and finally got it to build with my make of citadel. The existing make file did not include the options to build the recovery part. If I rebuilt it with configure, it worked. I ended up with the following for my dev version. I have a stubbed out library to check for memory leaks that I used in the db build, so that is the reason for the build order to get the header files into the ctdlsupport directory early because I delete the db files in ctdlsupport before building so it will not use a previous db versions header files. cd $(DB_DIR)/build_unix ../dist/configure make clean make install_include make make install_lib make install_utilities If you are not having the db tool errors, it is just the way I build it, but since EasyInstall did the same for me, might need to be fixed, no matter how it is built.
[Citadel Development] Database Recovery Tools
My database got corrupted and I tried to run the db_ tools and I got an error about the recovery not being compiled in. I did an easy install and the same thing. Can someone verify that db_verify does/does not work. Other tools had same error message. I had to go into the database build and run configure to generate the Makefile that would build it so it work work.
[Citadel Development] Re: Merge Request Approval: IMAP memory issues with use of ConstStr
Probably a role thing.
[Citadel Development] Re: Merge Request Approval: IMAP memory issues with use of ConstStr
I do not have permissions to check in. No button to merge and my icon shows "Cannot merge". I am logged in. > Mon Jul 31 2023 06:25:58 PM EDT from IGnatius T Foobar Subject: Re: Merge >Request Approval: IMAP memory issues with use of ConstStr > >Sorry about that, it just got away from me. I approved it. Looks good. > > > >
[Citadel Development] Re: Merge Request Approval: IMAP memory issues with use of ConstStr
This is still waiting for approval. Do you want me to bring the branch up to the head or anything else?
[Citadel Development] Re: Merge Request Approval: IMAP memory issues with use of ConstStr
While looking at the use of ConstStr, it was interesting how it was being used. "Const" is definably not the description to use. It really is just a StrBuf where the pointer does not need to be freed. All sorts of code change the pointer string and really did not check what it is doing. Probably worth replacing the use in our copious spare time lol.
[Citadel Development] Re: Approval Request: Memory_Leak_SmtpClient
Forgot to say that I was not logged in either and that was what was causing the check-in. I did not notice since I had access to everything. Strange
[Citadel Development] Merge Request Approval: IMAP memory issues with use of ConstStr
Had a coredump that was related to IMAP parameters and access to ConstStr. Not sure why the parameter was empty, but a lot of code accessed ConstStr pointers without checking for zero length and pointer was null and could coredump, so went through and added checks.
[Citadel Development] Re: Approval Request: Memory_Leak_SmtpClient
I am changing to updating the records on they fly rather than upgrading, so the version number is really not that important. New records use a version signature and don't compare to the citserver version so no conflict.
[Citadel Development] Re: Approval Request: Memory_Leak_SmtpClient
Tried to create a new Merge Request for a different branch and when I click to create it, the page just spins and never creates it. Something is up with GitLab > Thu Jul 20 2023 02:28:36 PM EDT from HarlowSolutions Subject: Re: >Approval Request: Memory_Leak_SmtpClient > > > >Either I do not have permission to merge, or something is wrong with >GitLab. When I bring up the Merge Request, the left menu has a bunch of >spinning updates that never complete. I was able to assign the merge to >myself, but that did not change anything, except there is a warning on my >icon that says "cannot merge". > > > >
[Citadel Development] Re: Approval Request: Memory_Leak_SmtpClient
Either I do not have permission to merge, or something is wrong with GitLab. When I bring up the Merge Request, the left menu has a bunch of spinning updates that never complete. I was able to assign the merge to myself, but that did not change anything, except there is a warning on my icon that says "cannot merge".
[Citadel Development] Approval Request: Memory_Leak_SmtpClient
There is a memory leak in the client. An unused variable is being malloc'ed and never freed.
[Citadel Development] Re: Master: citserver coredump on Shutdown
After I finish up the IMAP Flags, I should have some time to contribute to the database. I was initially thinking of SqlLite. The trick is what database schema we want to support. I was looking at keeping with the key/value concept and just replace database.c. I have an idea of the schema for the database that will be efficient on database size.
[Citadel Development] Re: Master: citserver coredump on Shutdown
Pretty consistent for me. I put a check in the Berkley code to work around, but not sure of the root cause either.
[Citadel Development] Re: Merge Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags
Duh. Button next to Comment button. I closed.
[Citadel Development] Re: Merge Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags
I just realized that a Seen message set of *:2099348686 means all messages have been seen. Were all your messages unseen before the upgrade? I do ignore Visit records when upgrading that look bad. If they got skipped, the message set would be blank and all messages would show as unseen after the upgrade. Can you check in the log to see what the message says about the conversion. Should show a count of any records were invalid/skipped. > Wed Jul 05 2023 01:48:13 PM EDT from HarlowSolutions Subject: Re: Merge >Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags > > > >I was trying to figure out how to reject the request myself. Does not look >like you can. I Googled and others were talking about how GitLab was >lacking that option. If you can figure out how to reject or delete it, I >will not be offended unless you don't tell me how :-) Otherwise I will just >update the branch and I think the request will get updated. > >Thanks for the string. I will play with it. In my testing, the upgrade >worked on Seen, but not very extensive examples. > > > >
[Citadel Development] Master: citserver coredump on Shutdown
When I build from master now, when I shutdown citserver, it core dumps right after closing the 0d database. Since I had an issue where webcit was core dumping and was not re-creatable by anyone else, I am wondering if this is just me. It looks like the Berkley code is passing a NULL to a memory free routine that does not check for NULL. Anyone else having a problem? If not, I will look into how I build the server.
[Citadel Development] Re: Merge Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags
I was trying to figure out how to reject the request myself. Does not look like you can. I Googled and others were talking about how GitLab was lacking that option. If you can figure out how to reject or delete it, I will not be offended unless you don't tell me how :-) Otherwise I will just update the branch and I think the request will get updated. Thanks for the string. I will play with it. In my testing, the upgrade worked on Seen, but not very extensive examples.
[Citadel Development] Re: Merge Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags
Thanks for the feedback. My testing only had a small database, so 10 min is rather bad. I think switching to doing the conversion when accessed is the best idea. It is simple since that is how I did it before I switched to the upgrade. I only have to add some code to one function. I am going to look into why the conversion did not work to make sure it is not an error that will occur when I do it on demand. I would be interested in the Seen message set string that caused it to fail. May be the pattern that is not being processed correctly. I will update the branch and submit a new request when I am ready.
[Citadel Development] Re: Merge Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags
No rush on the review. I had to put the code aside for a while (pesky day job :-) and had to write some tests to try to hit most code paths. I tried not to change too much existing code since the change is pretty big to begin with. I only tried to rewrite things that affected performance or optimize using the new functions. I tried to use the profiler (-pg) but was not able to get citserver to output the profile file, so I really have not tested before/after. If you know how to get the profiler working, I would be interest in seeing where we can optimize things for the biggest bang for the buck.
[Citadel Development] Re: Long term plans for phase-out of Berkeley DB?
If you want to stay with index/data for the database, I think I might go back and see if I can finish up a database.c replacement with MySql. What I don't have is the statistics for a large email server (mine is very small). I wrote a small C program that went through all the Berkley databases and found the min/max sizes for the indexes and data so I could see what the best implementation would be. If you have a large db, I can send you the program just to get the stats (unless you have dome this before). For memory leaks, I have been dealing with detecting them for years for a variety of companies. I found the best for C was to ifdef allocation/checking code and only use for testing. I used macros to replace the malloc functions with some header changes so you can activate your own routines without even having to edit all the allocation function calls. Then just have the checking functions allocate more memory than requested and put signatures before and after the allocations so you can track leaks and detect memory over/under runs. Slows things down a bit, but very good at identifying memory issues when testing.
[Citadel Development] Re: Long term plans for phase-out of Berkeley DB?
Just to chime in, the database.c code is very clean. The only problem I have had with Berkeley is that if the code or system crashes, most of the time, the database is corrupted and recover does not help sometimes and I have to revert to a backup. The code has become a lot more stable so not as big an issue anymore for me. I did think about making a drop in replacement for database.c using MySql. I started to play with it with the concept of key tables and having a set of tables with different sized blobs so you could store data records across multiple tables to optimize database size. Did not get to the point where I could compare performance with Berkley, but it looked promising. Optimally, not using indexes and rewriting all the code to use MySql tables and features would probably be the best, but a lot of work
[Citadel Development] Merge Request: Support_IMAP_Flagged_Deleted_Draft_Recent_flags
Touched a lot of files and complete rewrite of Visit support functions. Most of the logic centralized in user_ops.c to keep it clean and tried to optimize code and performance. If you have any questions let me know. I left merge request unassigned. If you approve without any changes, I will plan to approve the merge as you said. Building and Testing: 1. This version will upgrade the database records and the database will no longer be reverse compatible. Recommend copying your database and restoring after testing. 2. I changed the REV_LEVEL for citserver to 981 and my Visit record is also 981. When generating the new release with do-release.sh, you will need to change the REV_LEVEL back to 980. If you release before merging the Visit code, the Visit version number will also have to change.
[Citadel Development] Merge Request: Webcit_Coredump_StrBufQuotedPrintableEncode
Not sure if regression, but if you try to sent a message with a blank body, webcit coredumps. Just checked for empty message text and skipped trying to output.
[Citadel Development] Re: New Developer Questions
That is not the branch that was merged. It is the new one to actually add support for all the flags. I ran into some issues testing so I am working on it locally. Should be updating the branch and submitting soon. Adding the flags is a big change (hopefully for the better), but touches a lot of files.
[Citadel Development] Re: New Developer Questions
I am completely new to Git and commits, so I did not realize there was a standard :-P Thanks for the feedback and access. I'll now shut up and code :-)
[Citadel Development] Re: New Developer Questions
Ok. I have gotten my feet wet, so now I have a couple more questions. I am trying not to be annoying, but I seem to be running into things that I am not sure how to handle. Just let me know if I should shut up and code :-) As you have seen, I have been creating rather long, detailed commit information. Do you want this type of detail in the commit or do you just want a smaller summary? I am writing like I am providing details for a code reviewer. If you don't want all the detail in the commit, I could add a message here with the details when I submit the merge request, or maybe there is something in GitLab we can use? Is there a list of issues or enhancements that we can review and maybe work on? I am willing to put in some effort to help with it. This might be something that we could use the Issues in GitLab for.
[Citadel Development] Re: Daily commit digest for Citadel (CDB_VISIT)
Did not mean to attach the patch in the last message. It looks like Merge requires you to create a branch to merge. Do you have any naming conventions you would like to use for branch names or is there a different way you want to submit code? > Thu Jun 01 2023 09:32:30 PM EDT from HarlowSolutions Subject: Re: Daily >commit digest for Citadel (CDB_VISIT) > > > >GitLab looks nice. I figured out how to clone and I created a patch. I >will go and try to use merge. > > > > From 37f45c6a1e405f31f79a24cd03eb9090e707cf84 Mon Sep 17 00:00:00 2001 From: Harlow Solutions Date: Thu, 1 Jun 2023 21:10:05 -0400 Subject: [PATCH] IMAP_Flag_Clearing_and_User_Purge_Vist_Records MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed imap_append() from clearing all flags on messages and avoid purge_user() from purging the wrong Visit records. imap_misc.c: imap_append() was trying to append the flags after the room had been restored to the original room, so all flags in the room were being cleared. Moved imap_do_append_flags() before the room was changed back. user_ops.c: purge_user() was assuming that the first part of the key for Visit records was the user number. Actually is the room number so was removing records where the room and user numbers matched. A solution is to go through all the Visit records and delete, but not worth it. Auto-Purge will clean up the records later. Future: When expanded IMAP flagging is added, we will move the user number to the top of the key and reactivate the existing method. ctdl3264_prep.sh: Build fix to support âtailâ command version differences for + option. CentOS/Rocky Linux needs -n option added for command not to error. Now tests command and sets the proper option to work. --- citadel/server/modules/imap/imap_misc.c | 7 +++ citadel/server/user_ops.c | 6 +- citadel/utils/ctdl3264_prep.sh | 8 +++- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/citadel/server/modules/imap/imap_misc.c b/citadel/server/modules/imap/imap_misc.c index 8d785cb61..e2da23c4f 100644 --- a/citadel/server/modules/imap/imap_misc.c +++ b/citadel/server/modules/imap/imap_misc.c @@ -384,6 +384,9 @@ void imap_append(int num_parms, ConstStr *Params) { new_msgnum = CtdlSubmitMsg(msg, NULL, ""); } if (new_msgnum >= 0L) { + if (IsEmptyStr(new_message_flags)) { +imap_do_append_flags(new_msgnum, new_message_flags); + } IReplyPrintf("OK [APPENDUID %ld %ld] APPEND completed", GLOBAL_UIDVALIDITY_VALUE, new_msgnum); } @@ -405,8 +408,4 @@ void imap_append(int num_parms, ConstStr *Params) { /* We don't need this buffer anymore */ CM_Free(msg); - - if (IsEmptyStr(new_message_flags)) { - imap_do_append_flags(new_msgnum, new_message_flags); - } } diff --git a/citadel/server/user_ops.c b/citadel/server/user_ops.c index e5eddf069..7c2d79b93 100644 --- a/citadel/server/user_ops.c +++ b/citadel/server/user_ops.c @@ -871,7 +871,11 @@ int purge_user(char pname[]) { PerformUserHooks(&usbuf, EVT_PURGEUSER); // delete any existing user/room relationships - cdb_delete(CDB_VISIT, &usbuf.usernum, sizeof(long)); + // Commenting out since was assuming the user number was + // at the top of the index. Room number is actually first. + // Auto-Purge will clean up the records later, so not worth + // scanning all the records here. + //cdb_delete(CDB_VISIT, &usbuf.usernum, sizeof(long)); // delete the users-by-number index record cdb_delete(CDB_USERSBYNUMBER, &usbuf.usernum, sizeof(long)); diff --git a/citadel/utils/ctdl3264_prep.sh b/citadel/utils/ctdl3264_prep.sh index 8a1cffc9d..cb3a5bbd2 100755 --- a/citadel/utils/ctdl3264_prep.sh +++ b/citadel/utils/ctdl3264_prep.sh @@ -6,11 +6,17 @@ # Source our data structures from the real live working code SERVER_H=server/server.h +tail_opt= +tail +1 ${SERVER_H} > /dev/null 2>&1 +if [ $? != 0 ] ; then + tail_opt=-n +fi + # Generate the "32-bit" versions of these structures. # Note that this is specifically for converting "32-bit to 64-bit" -- NOT "any-to-any" convert_struct() { start_line=$(cat ${SERVER_H} | egrep -n "^struct $1 {" | cut -d: -f1) - tail +${start_line} ${SERVER_H} | sed '/};/q' \ + tail ${tail_opt} +${start_line} ${SERVER_H} | sed '/};/q' \ | sed s/"^struct $1 {"/"struct ${1}_32 {"/g \ | sed s/"long "/"int32_t "/g \ | sed s/"time_t "/"int32_t "/g \ -- 2.39.3
[Citadel Development] Re: Daily commit digest for Citadel (CDB_VISIT)
GitLab looks nice. I figured out how to clone and I created a patch. I will go and try to use merge. 0001-IMAP_Flag_Clearing_and_User_Purge_Vist_Records.patch Description: Binary data
[Citadel Development] Re: Daily commit digest for Citadel (CDB_VISIT)
I am testing a complete replacement for the Visit handling. Looking good so far, but since we don't have a set of tests to check for regression, I am just running it and trying to hit as many code paths as I can. In order to add all the IMAP flags, I had to pretty much rewrite the related code. I fixed a couple of defects with the existing code that I am just going to include with the new flag support. Small but if you want me to submit separate, let me know. The best one is that when you purge a user, it assumes that the associated Visit record's user number is the top of the key. It is actually the third, so it is trying to delete all Visits with a room number that matches the user number :-P
[Citadel Development] Design Question: Implementing Extended Flag Support
I am working on the changes to implement support for making the Flagged, Deleted and Draft flags persistent like the Seen and Answered flags. Instead of just adding more large buffers to the Visit structure, I have implemented them by removing the buffers from the structure and storing all the message set strings after the structure in the database record. The strings are just copied one after the other, so only the string values are saved. Should greatly reduce the database space used by the views. On loading a View record, a list of StrBufs stores the values until they are written to the database. This should greatly reduce the database space used to store the records for even the current records (Seen and Answered) which take up 8K in buffers and putting the message list strings into dynamically allocated StrBuf links is pretty efficient. I also added a version number to the view structure to make it easier to change the structure in the future, and was required since my version of the structure is smaller than the original and I could not use the size of the database record to determine the version like the existing code does since my version’s database record size depends on the total size of the message set strings which could match the old version’s size. Now to the issue: How do you want to support the change to the structure and falling back to older versions of the mail server. My implementation reads the database record and will determine if the record contains the old Visit structure or the new (based on the version number in the new structure which will never match the binary value in the older versions). If old, it converts it to the new, and the new is used in the rest of the code and the new version replaces the old when written. The issue with this is that once you start the server with this code, you cannot fall back to an older version of the server since the old structure is not compatible with the new structure. If we need to support falling back to older version(s) for a while (e.g. until the next release or two), I can only think of one way to implement it without maintaining multiple records: * Keep the old version in the top of the database record and add the new one after. When reading, the new version would detect the appending new structure and use it. The old code would just read the old version of the structure and the new version would be lost on write. Downsides: * Database records will remain to be about 8K (old version size) plus the new persistent flags (would not be in large buffers at least). * If you fall back, you lose the persistent data of the new flags when going back to the later. What is the development philosophy on falling back? Do we need to maintain the ability to fall back? How many releases? If we need to fall back, what option should be implemented (1, 2 or another idea)? Both ideas are a bit ugly. Also, I only update the database records to the new version if they are saved, so a mixture of old and new records will be in the database unless we go through every room and convert. Is this type of thing ever done (one time upgrade)?
[Citadel Development] Patch for Review: Bugfixes to message flag handling
I have been working on implementing message sets for Flagging, Draft, etc. Noticed flags were getting set wrong in Outlook and in WebCit. I thought I had introduced the bug(s). Finally realized that the bugs were in the release and had nothing to do with my code. Spent way too much time debugging my code :-) Only one line changes but wanted to keep separate from other work. serv_imap.c (citserver)/roomops.c (webcit): bugfixes to message flag handling (Phil Slack) serv_imap.c: IMAP message flags are not initialized when loading messages. imap_set_seen_flags() is passed the index of the first message to process. It is zero based, but the function error check rejects a value of zero, so the flag for the messages in the room updated. Changed check to allow zero. roomops.c: Mail messages in a room are set to Seen when just viewing the room. A mailbox room view can be a mailbox view or a json list view. dotgoto() did not handle when set to json and set all the messages to Seen. Changed to handle a json view the same as a mailbox view. 0001-serv_imap.c-citserver-roomops.c-webcit-bugfixes-to-m.patch Description: Binary data
[Citadel Development] Patch for Review: Citadel cprintf() Error Handling
Attached is a patch to fix the cprintf() function in Citadel. It was not handing truncation properly and could output invalid data. 0001-Citadel-cprintf-does-not-handle-string-truncation-pr.patch Description: Binary data
[Citadel Development] EasyInstall: Citadel Service Startup Issue
I am not sure where the EasyInstall script is, but there is an issue on at least on CentOS and Rocky Linux when systemd starts citserver. I am not sure if it is an issue on other Linux systems, so someone may know if what I did to fix it is portable. citdel.service uses a target of network.target. I found there is a race condition when citserver is starting up where the network startup is not ready and citserver fails to open the listening ports. I had to change network.target to network.online.target to ensure the network is up before citserver is run.
[Citadel Development] Re: Patch for Review: Fix LibCitadel Strip Functions
Phil Slack
[Citadel Development] Patch for Review: Fix LibCitadel Strip Functions
Main issue is stripout() looses a character when stripping. Reworked both stripout() and stripallbut() to be consistent in handling strips. Actually runs faster than original code. For multiple boundaries, stripout() mangled the resulting string and stripallbut() just returned the original. I made consistent by looking for the first and last occurrence of the boundary character. If you think the boundaries should be handled differently, let me know. Really should not happen in the real world very often, but should be consistent. There is also a flush of stderr in stripallbut() which I am not sure why is there. stderr is not buffered, so flushing does not do anything, but that is out of the scope of this issue, so I just left in. -- Fix LibCitadel stripout() function removing characters. Make strip functions boundary handling consistent. libcitadel/lib/tools.c stripout() Strips too many characters. Causes incorrect From address on inbound and probably other places. stripout(“Foobar”) = Fooba. Should be Foobar Mangles multiple boundaries. Should strip using outer boundaries. stripout(“ABC()(DEF)()GHI”) = ABC()(DEFGHI. Should return ABCGHI. stripallbut() Handle outer boundaries like stripout() stripallbut(“ABC()(DEF)()GHI”) returns unchanged. Should return )(DEF)( Code does a stderr flush. Not sure why, but left it in. string_trimlen() Removed function. Not used anywhere in the code and less efficient than string_trim() 0001-Fix-LibCitadel-stripout-function-removing-characters.patch Description: Binary data
[Citadel Development] Re: New Developer Questions
The bootstrap was the part I was missing. Thanks. For webcit, I am glad to hear that it is getting rewritten. At one point, I tried to fix the calendar so it would work as an internet calendar in Outlook, but after a bunch of work, it was not worth it. Got it talking to Outlook, but then I found there were bugs in the iCal library and just decided it was not worth spending any more time on it. I ended up using iCloud to share calendars. One thing I will submit a patch for is the recipient handling. Some of my users caused core dumps because of long addresses. It looks like the address length is limited to 256 and the total recipient line is limited to 900 characters. I think these were probably from the standard, but I have found that many mail servers violate the standard. Is there any reason to limit these in the server? For my implementation, I just bumped up both to handle SIZ (4096) throughout the code and have not seen any issues. Any reason I should not be allowing it? If there is, I can just add the code to avoid the core dump. Out of curiosity, why are the Makefiles built for libcitadel and webcit, but are checked in for citadel and textclient?
[Citadel Development] Build Broken
The build is currently broken with the last changes. citadel/bootstrap is still referencing citadel.h. Should be citadel_defs.h.
[Citadel Development] Re: New Developer Questions
Submitting Code: What you said makes very good sense. To try out submitting code, I cloned down the citadel release, created a branch and made some updates to fix some WebCit page issues and cleaned up a little formatting. Seemed like a simple change to submit with low impact for a first try. I created a patch from the branch (attached). Is this what you were expecting for submitting changes? New Features: The Flagging and Aliasing were just examples. The Aliasing is a big help to my server, but I figured may not be considered a useful feature to others. It is a bit tricky to implement and as you noted, you have to be careful about user names. It is fun when you get to tell a company like Fidelity Investments that their email list has been stolen and you have the proof :-) The flagging is most likely useful. I added support for Draft, Flagged, Deleted and Recent. After reviewing the standards I noticed that Seen was not quite implemented properly so I fixed that while implementing the others. I’ll write up details on it and send it to you. Building Code: The main issue I ran into was trying to build the server from the citadel repository. I normally have just copied the build after an easy install and then made my changes on top of it so all I had to do is run Make. With the citadel repository, I tried to create the configure scripts and then run them to create the make files, but ran into some missing files and script errors. I am compiling on Rocky Linux. I assume that developers are not running autoconf, etc. from just a citadel repository clone or to build easy install. What is the proper process for building from the repository? 0001-WebCit-Fix-HTML-errors-and-clean-up-page-output.patch Description: Binary data
[Citadel Development] Re: New Developer Questions
Thanks. Looking forward to it.
[Citadel Development] New Developer Questions
I want to start contributing to the Citadel project as a developer, but I want to understand the developer process for submitting changes and testing. I have been using Citadel as a small business server for a few years now and have fixed bugs and limitations and also added a few features for my own use over time. I don't want to start committing code and find out that I should have been doing it a different way or break anyone's development. Is there any document covering how-to and best practices? If not, here are my initial questions: Git I have never used git before, but have cloned down the source and did a little reading. I think all I need to do is git add my changes and then commit. What is the best practice? Create branches? Doesn't matter? Do you have an example commit command so I know all the options that I should be using. Any standards? Testing Is there any test suite for Citadel? I do my own testing, but I mainly support Outlook and IMAP, so I am not sure how to test for regression in other protocols and clients. How are committed changes tested before release? New Features If I have an idea for a new feature, how do I find out if you want it implemented and the best way to integrate? Example: I implemented Flagging in IMAP by expanding on the Visit structure. Example: I expanded aliasing to allow users to use email addresses appended by anything to go to their account without explicitly adding an alias (e.g. User is myacct. Email like myaccts...@example.com, myacctb...@example.com, etc will be redirected to myacct). Has been very useful for anti-spamming and identifying who gave away/lost and email address. Supported sending using the alias also.