Hello, On ketvirtadienis 13 Rugpjūtis 2009 16:10:25 David Kalnischkies wrote: > Hello Modestas Vainius, > > > the patch attached is trivial and probably needs only a couple of minutes > > to review. Could this be done any time soon? > > I am not an apt maintainer, so this is not an official review, > but i have a little comment on your patch, beside my comment that it is > not always a good idea to react in such an angry fashion just because > nobody respond directly on this minor issue, it doesn't help in getting > the review faster or only delay it further... Sorry if it sounded harsh, that was not intentional. It was rather "ping" about (imho) trivial bug than angry complaint. > -- and btw: personally > i would wish that squid response with a "304 Not Modified" or so... > Just because it is allowed doesn't mean it should be done... Sure, but squid version is typically out of the user control... And squid does not do anything invalid ... > Anyway: Your patch uses the %n conversion specifier, > the manpage of sscanf says the following about this directive: > "The C standard says: 'Execution of a %n directive does not increment the > assignment count returned at the completion of execution' but the > Corrigendum seems to contradict this. Probably it is wise not to make > any assumptions on the effect of %n conversions on the return value." > (scanf (3) -> Description -> Conversions ~ 2008-07-12) > > At least this means it maybe increment the counter or not, so in the if > we would need to check for sscanf < 4 instead of sscanf != 4 to be able > to work under each standard conform implementation of the sscanf method. > > This said, i would suggest to not use the %n at all and just remove the > space before the last directive. So this space which delimits the Result > and the Code is then included in Code -- but Code is not really used anyway > as far as i can see (just in one little error message and an extra space > would not harm here i guess) - or if this really harms anybody we could > simply remove the heading space in an added else part of the if's... > > > What do you think? I don't like that %n very much either, especially if its documentation is contradicting as you say. Actually, any solution is fine for me as long as the false failure is gone. I'm not an APT expert myself and when writing the patch I simply didn't want to change semantics of 'Code' and 'Result'. %n was the shortest (but apparently not the best) thing I could come up with. > P.S.: I will also ask Michael Vogt about it then he is back online > next week (i hope), just be a bit more patient, everything will be fine. ;) Thank you. Much appreciated. -- Modestas Vainius <modestas@vainius.eu>
Attachment:
signature.asc
Description: This is a digitally signed message part.