On Mon, Nov 27, 2017 at 11:06 PM, David Steele <da...@pgmasters.net> wrote: > On 11/15/17 10:09 PM, Michael Paquier wrote: >> On Thu, Nov 16, 2017 at 9:20 AM, David Steele <da...@pgmasters.net> wrote: >>> For this patch at least, I think we should do #1. Getting rid of the order >>> dependency is attractive but there may be other programs that are depending >>> on the order. I know you are not proposing to change the order now, but it >>> *could* be changed with #2. >> >> read_backup_label() is a static function in the backend code. With #2 >> I do not imply to change the order of the elements written in the >> backup_label file, just to make the way they are parsed smarter. > > My point is that the order *could* be changed and it wouldn't be noticed > if the read function were improved as you propose. > > I'm not against the idea, just think we should continue to enforce the > order unless we decide an interface break is OK.
I still don't quite understand here. The order the items are read does not cause a backward-incompatible change. True that there is no reason to change that either now. >>> Also, I think DEBUG1 would be a more appropriate log level if any logging is >>> done. >> >> OK, the label and time strings can include spaces. The label string is >> assumed to not be bigger than 1028 bytes, and the timestamp is assumed >> that it can get up to 128. So it is possible to use stuff like >> fscanf("%127[^\n]\n") to avoid any buffer overflow problems on the >> backend. If a backup_label is generated with strings longer than that, >> then read_backup_label would fail to parse the next entries but that's >> not really something to worry about IMO as those are only minor sanity >> checks. Having a safer coding in the backend is more important, and >> the new checks should not trigger any hard failures. > > I have tested and get an error as expected when I munge the backup_label > file: > > FATAL: invalid data in file "backup_label" > DETAIL: Timeline ID parsed is 2, but expected 1 > > Everything else looks good so I will mark it ready for committer. Thanks. This maps what I saw. -- Michael