Hi Mike, On Fri, May 29, 2015 at 08:57:03AM +0000, Mike Morozov wrote: > Hi, > > I work for 51Degrees, we provide a device database and detection software to > get device information from the device's HTTP headers. We have made a > converter that can take a User-Agent header and create another header filled > with device information to be sent to backend servers.
Great, that's very welcome and in the trend these days :-) (...) > The following is an example of the settings for Pattern. > > 51degrees-data-file > '51D_REPO_PATH'/data/51Degrees-Lite.dat > 51degrees-property-name-list IsMobile ScreenPixelsWidth > ScreenPixelsHeight > 51degrees-property-seperator , > > The converter has the following config: > http-request set-header X-51D-MobileWidthHeight > %[req.fhdr(User-Agent),51d(IsMobile,ScreenPixelsWidth,ScreenPixelsHeight)] > > Converters take a maximum of 5 properties that were initialised in > 51degrees-property-name-list and will return a string of values in the same > order as the properties were supplied. OK, that sounds fairly easy to use, thanks for the explanation. > I hope our code is up to the HAProxy standard. We're happy to make any > changes needed. I took a quick glance at the patches, I have some comments but nothing blocking so I definitely think we must merge this with 1.6 even if further work is needed. First point is that some of your patches seem to fix issues on some of the previous patches, for example this one : From d2d0102e2d4a1de4b733bfa0346d973594ae2f90 Mon Sep 17 00:00:00 2001 From: Thomas Holmes <[email protected]> Date: Thu, 21 May 2015 16:09:39 +0100 Subject: [PATCH 13/14] Fixed error in 51Degrees pattern that resulted in random values being returned when a non existant property was requested. Normally it's better to provide only a "clean" series, by fusing the fixes into where they belong to (ie: "git rebase -i" and use fixups). If this is something you're not used to, we can help you with this, do not worry. To go a bit further, it seems that the series first attempts to introduce one way to use the lib and that the author changed his mind after that and reworked this, explaining the additions/removals to README and some parts of the code. Then better fuse these patches as well so that we merge the final design only and not the intermediary ones that will never be used. I noticed another issue which we'll have to fix, I suspect that several persons worked on the same code with editors configured differently, because the indents are completely mangled. Sometimes there are tabs, sometimes 4 spaces, sometimes 8, making me think that someone has a tabstop set to 4 but sometimes uses spaces instead of tabs. This gives something like this at a number of places : @@ -2215,8 +2215,7 @@ static int fiftyone_degrees_trie(const struct arg *args, // try to find request property in dataset. propertyIndex = fiftyoneDegreesGetPropertyIndex(args[i].data.str.str); if (propertyIndex > 0) { - chunk_appendf(tmp, "%s", fiftyoneDegreesGetValue(deviceOffset, propertyIndex)); - break; + chunk_appendf(tmp, "%s", fiftyoneDegreesGetValue(deviceOffset, propertyIndex)); } else { chunk_appendf(tmp, "%s", noData); I'm seeing something of no importance, we don't use CamelCase when naming identifiers in haproxy, and I'm seeing that a few fields added to the global struct use that scheme. I'm realizing that the types provided by your lib do this so for the types you have no choice, but as much as possible we should avoid this to maintain some consistency with the rest of the code. Thus as you can see, all this is fairly trivial and non-important, which is a good news :-) I'd like to issue dev2 this week-end and mark the feature freeze since we said it was at the end of May and we're already late in this month. If you think you can apply some cleanups regarding the comments above before this week-end, that would be highly appreciated. Otherwise, don't worry, I consider that there's nothing wrong there and the code clearly qualifies for merging, even if we have to do so next week or so. Thank you! Willy
