Re: [PATCH] wurfl device detection fixes
Hi Massimiliano, On Tue, May 21, 2019 at 05:23:16PM +0200, Massimiliano Bellomi wrote: > Hi All. > > Here attached you may find a new set of patches related to WURFL module > that should address Christopher and Willy suggestion. Thank you, I've applied them. I tried to add some info to the commit messages where I was able to figure the purpose of the change by looking at the code. I also added one extra patch to silence the module when it is not configured, because till now it used to emit warnings upon startup all the time, which is very confusingg and prevents from sanely integrating it in a CI process. Thanks, Willy
Re: [PATCH] wurfl device detection fixes
Hi Massimiliano, On Fri, May 17, 2019 at 08:17:34AM +0200, Massimiliano Bellomi wrote: > Thank you Christopher. > > Is it fine for you if I send a 9th patch which fixes the 3rd one ? ( ...so > patches still be applied in sequence from 1 to 9, patch 3 included ) No, please never submit a patch that is known wrong and needs to be fixed by another one. It is important because developers spend a great share of their time dealing with complex bugs that require lots of bisects, which means commits must have the most correct behaviour possible. The worst thing (and it still occasionally happens) is when a bisect operation lands on a commit which doesn't even compile... I'm never going to blame anyone for integrating bugs or stuff that doesn't compile, it happens to all of us from time to time. Sometimes you perform a last minute cleanup after your tests, you get distracted, you submit and it's broken. OK. But distributing a bug on purpose, no, please never do that. Thus please fix your original patch to include this fix. Also, please make your commits a bit more descriptive, think about the "what, why, how" principle I often ask for (ie what this change does, why it does it, and how you chose to do it). And when it's a bug, it's "what is the problem, why does it do this, and how do you fix it". Actually reviewing patches that contain no info takes a lot of time because one has to figure based on the patch what was attempted to be done. Usually a well described commit is immediately applied (or commented) and a not well one can rot for several days until someone finds the time to do it, and has the willingness to edit the commit message to place in it all what they figured so that this info is not lost for the future. When in doubt, think you're trying to sell me your patch series and your role is to convince me to buy it. It's not far from the reality anyway since any patch adds a little bit to the maintenance effort. Thanks, Willy
Re: [PATCH] wurfl device detection fixes
Thank you Christopher.
Is it fine for you if I send a 9th patch which fixes the 3rd one ? ( ...so
patches still be applied in sequence from 1 to 9, patch 3 included )
Regards
-Max
On Thu, May 16, 2019 at 9:49 AM Christopher Faulet
wrote:
> Le 14/05/2019 à 11:56, Massimiliano Bellomi a écrit :
> > Hi All.
> >
> > Here attached you may find a set of patches related to WURFL module.
> >
> > Patches from 0001 to 0004 should implements Christopher's last
> > suggestions/issues.
> >
> > * segfault when I try to retrieve an unknown data (I mean not listed
> > in wurfl-information-list).
> > * the channel validity must be checked calling the macro
> > CHECK_HTTP_MESSAGE_FIRST().
> > * the function ha_wurfl_retrieve_header() is not HTX aware
> > * It could be cool to call ha_wurfl_retrieve_header() from the dummy
> > library, in wurfl_lookup().
> >
> > Patches from 0005 to 0008 corrects some small issues found during this
> > activity.
> >
> > Thank you in advance for your feedback. ( ...and thank you Willy for
> > your clarifications on send_log() )
> >
>
> Hi,
>
> There is a problem with the third patch ("MINOR WURFL makes
> ha_wurfl_retrieve_header() HTX aware"). It is probably my fault because
> my previous feedback was not so explicit. ha_wurfl_retrieve_header()
> must be HTX aware. But the samples too, ha_wurfl_get_all() and
> ha_wurfl_get().
>
> In HTTP sample fetches, we should switch on the proxy option
> PR_O2_USE_HTX to call the right prefetch function. This way:
>
> if (smp->px->options2 & PR_O2_USE_HTX) {
> /* HTX version */
> struct htx *htx = smp_prefetch_htx(smp, chn, 1);
>
> if (!htx)
> return 0;
> ...
> }
> else {
> /* LEGACY version */
> CHECK_HTTP_MESSAGE_FIRST(chn);
>
>
> }
>
> For other patches, it seems to be good.
>
> --
> Christopher Faulet
>
--
Massimiliano Bellomi
Senior Software Engineer
Scientiamobile Italy - massimili...@scientiamobile.com +39 338 6990288
Milano Office : +39 02 620227260
skype: massimiliano.bellomi
Re: [PATCH] wurfl device detection fixes
Le 14/05/2019 à 11:56, Massimiliano Bellomi a écrit :
Hi All.
Here attached you may find a set of patches related to WURFL module.
Patches from 0001 to 0004 should implements Christopher's last
suggestions/issues.
* segfault when I try to retrieve an unknown data (I mean not listed
in wurfl-information-list).
* the channel validity must be checked calling the macro
CHECK_HTTP_MESSAGE_FIRST().
* the function ha_wurfl_retrieve_header() is not HTX aware
* It could be cool to call ha_wurfl_retrieve_header() from the dummy
library, in wurfl_lookup().
Patches from 0005 to 0008 corrects some small issues found during this
activity.
Thank you in advance for your feedback. ( ...and thank you Willy for
your clarifications on send_log() )
Hi,
There is a problem with the third patch ("MINOR WURFL makes
ha_wurfl_retrieve_header() HTX aware"). It is probably my fault because
my previous feedback was not so explicit. ha_wurfl_retrieve_header()
must be HTX aware. But the samples too, ha_wurfl_get_all() and
ha_wurfl_get().
In HTTP sample fetches, we should switch on the proxy option
PR_O2_USE_HTX to call the right prefetch function. This way:
if (smp->px->options2 & PR_O2_USE_HTX) {
/* HTX version */
struct htx *htx = smp_prefetch_htx(smp, chn, 1);
if (!htx)
return 0;
...
}
else {
/* LEGACY version */
CHECK_HTTP_MESSAGE_FIRST(chn);
}
For other patches, it seems to be good.
--
Christopher Faulet
