From: Sean M. <se...@ro...> - 2011-12-09 18:55:00
|
Hi all (especially Nathan!), I'm having intermittent USB-related crashes, and so have been looking at the libusb code for threading problems. I think I've found a few, but as I'm neither a libusb nor threading guru, I would be great if someone could review. I've also fixed a few warnings. 6 patches attached. Assuming they pass review, can we get these in the testing branch please? Thanks, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Segher B. <se...@ke...> - 2011-12-09 22:26:56
|
> I would be great if someone could review. Maybe if you send them as inline text instead of as base64 application/octet-stream... Segher |
From: Michael P. <mic...@gm...> - 2011-12-10 02:32:48
|
Sean McBride wrote: >> I'm having intermittent USB-related crashes, and so have been looking >> at the libusb code for threading problems. I think I've found a few, >> but as I'm neither a libusb nor threading guru, I would be great if >> someone could review. I've also fixed a few warnings. 6 patches attached. >> >> From dd8208acdd7c87a801639f729410710acb1d9969 Mon Sep 17 00:00:00 2001 >> From: Sean McBride <se...@ro...> >> Date: Fri, 9 Dec 2011 13:50:59 -0500 >> Subject: [PATCH 116/116] fixed clang static analyzer warning by making >> private function static >> >> --- >> libusb/core.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/libusb/core.c b/libusb/core.c >> index 6418301..d5eb4db 100644 >> --- a/libusb/core.c >> +++ b/libusb/core.c >> @@ -1680,7 +1680,7 @@ int API_EXPORTED libusb_has_capability(uint32_t capability) >> return 0; >> } >> >> -void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level level, >> +static void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level level, >> const char *function, const char *format, va_list args) >> { >> FILE *stream = stdout; >> -- >> 1.7.7 This function is used in files other than core.c. Regards, Michael |
From: Sean M. <se...@ro...> - 2011-12-12 16:56:41
|
On Fri, 9 Dec 2011 21:32:29 -0500, Michael Plante said: >>> -void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level level, >>> +static void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level >level, >>> const char *function, const char *format, va_list args) >>> { >>> FILE *stream = stdout; >>> -- >>> 1.7.7 > > >This function is used in files other than core.c. Quite right. OK, so how to fix this warning: core.c:1683:6: warning: no previous prototype for function 'usbi_log_v' [-Wmissing-prototypes] Declare a prototype in core.c? in libusbi.h? Conditionally tag it static depending on OS? Cheers, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Pete B. <pe...@ak...> - 2011-12-12 20:48:35
|
On 2011.12.12 16:56, Sean McBride wrote: > Quite right. OK, so how to fix this warning: > > core.c:1683:6: warning: no previous prototype for function 'usbi_log_v' [-Wmissing-prototypes] > > Declare a prototype in core.c? in libusbi.h? Conditionally tag it static depending on OS? I think the issue is that the core.c definition of usbi_log_v isn't behind #ifdefs, as its prototype is in libusbi.h (circa line 130). Surrounding it with: #if defined(_MSC_VER) && _MSC_VER <= 1200 #endif in core should address the issue. usbi_log_v is aimed purely at MSVC6. Regards, /Pete |
From: Michael P. <mic...@gm...> - 2011-12-13 01:25:18
|
Pete Batard wrote: >> I think the issue is that the core.c definition of usbi_log_v isn't >> behind #ifdefs, as its prototype is in libusbi.h (circa line 130). >> >> Surrounding it with: >> #if defined(_MSC_VER) && _MSC_VER <= 1200 >> #endif Adding extra #ifdef's can be ugly. Would it be easier just to move the prototype out of the #if that's already in libusbi.h? Also, usbi_log calls usbi_log_v in all versions, doesn't it? Michael |
From: Peter S. <pe...@st...> - 2011-12-13 15:41:04
|
Pete Batard wrote: > >> Yes, and it was found that MSVC6 compilation was broken there. > >> Did you miss that discussion? It was on the mailing list... > > > > I think I have. On the other hand if there is no ticket, no fix > > and no discussion for a couple of months then it can not be too > > important? > > A patch for mainline was sent less than a week ago (2011.12.07) and > reviewed/acked by Segher. I told you privately that you'd need an > MSVC6 patch about 1 month ago. And there was the public thread. Oh that's what you refer to. Is Ludovic's fix not good? //Peter |
From: Pete B. <pe...@ak...> - 2011-12-13 15:43:28
|
On 2011.12.13 15:40, Peter Stuge wrote: > Oh that's what you refer to. Is Ludovic's fix not good? I'm afraid Ludovic's fix has nothing to do with it. It had to do with "error C2065: 'LONG_PTR' : undeclared identifier; in poll_windows" and other stuff, that Ludovic's patch does not impact. Please see the updated patch in the 2011.12.07 MSVC6 [PATCH] thread. Regards, /Pete |
From: Pete B. <pe...@ak...> - 2011-12-13 15:47:11
|
On 2011.12.13 15:31, Peter Stuge wrote: > In case there is no feedback, nothing can be assumed. That's quite convenient to justify the induced integration delays we see in libusb, and which do not seem to occur much of anywhere else. Also, aren't you then "assuming" that no feedback equates not ready for integration. Personally, if you don't want to "assume OK", I'd see as part of maintainer's job (or gerrit when available) to ask for feedback on longstanding open and non ack'ed issues on a regular basis and try to make their numbers go down. Regards, /Pete |
From: Peter S. <pe...@st...> - 2011-12-14 03:35:14
|
Pete Batard wrote: > On 2011.12.13 15:31, Peter Stuge wrote: > > In case there is no feedback, nothing can be assumed. > > That's quite convenient to justify the induced integration delays Convenient has nothing to do with it. It is simply a fact that lack of feedback can never have a conclusive meaning in favor or against. It may be the very definition of unknown. Wiktionary says "whose value is to be found." > aren't you then "assuming" that no feedback equates not ready for > integration. No, but you are right that I assume something; the only reasonable thing; that no feedback equates noone having looked at the proposal or cared enough about it in order to comment on it. > Personally, if you don't want to "assume OK", I'd see as part of > maintainer's job (or gerrit when available) to ask for feedback I'd consider that babysitting, not something fitting in a group of adult developers. If someone cares about a change then they have to take action and contribute actively. If they don't take action I can only assume that they don't care too much about that change. (Either not reviewing it, or not bothering to feed back after review.) When I can contribute time then I review patches myself, rather than tell someone else to do it. I expect everyone else to do the same. I don't know how many times I have said that anyone and everyone is invited to do review, and it's getting old. Go for it. //Peter |
From: Pete B. <pe...@ak...> - 2011-12-14 12:04:21
|
On 2011.12.14 03:35, Peter Stuge wrote: >>> In case there is no feedback, nothing can be assumed. >> >> That's quite convenient to justify the induced integration delays > > Convenient has nothing to do with it. It is simply a fact that lack > of feedback can never have a conclusive meaning in favor or against. Well, it is pretty clear to me that lack of feedback conclusively means "against" as far as you are concerned. I know of no other project where the maintainer _delays_ the integration of simple patches that haven't received explicit review for weeks or months, and attempts to justify that delay by saying "if nobody has been acking it, then it probably means it should not be considered for integration yet". Patches do very much have some sense of urgency, as well as an expiration date. If you don't tackle the ones that can be tackled straight away, even if they haven't received an explicit review, you are asking for exactly the kind of trouble we've been seeing over and over again on this list, and, as you have very ironically demonstrated yourself, you are bound to forget everything about critical items, such as compilation being broken for a specific dev env... >> aren't you then "assuming" that no feedback equates not ready for >> integration. > > No, but you are right that I assume something; the only reasonable > thing; Here we go again. Apparently there's a universal "reasonable" thing to do, which, rather conveniently again, happens to be exactly the continuation of what you've been doing so far on a project that everybody agrees is dysfunctional. Still no questioning of your approach whatsoever. > that no feedback equates noone having looked at the proposal > or cared enough about it in order to comment on it. OK, so you do, in effect, would like every last patch submitted to this list to have an ack from somebody other than you, before you consider them integration material. I think this is a different process from what many of us here had in mind. At least it gives the rest of us an idea of what you actually expect, which I guess is some form of progress... >> Personally, if you don't want to "assume OK", I'd see as part of >> maintainer's job (or gerrit when available) to ask for feedback > > I'd consider that babysitting, not something fitting in a group of > adult developers. Except our project is dysfunctional and a backlog of non integrated patches keeps accumulating. I don't think the "let's keep doing what we've been doing" or "when we have gerrit it'll all get better" is a sane course of action at this stage. Something clearly doesn't work with the libusb integration process, and this even when patches are reviewed and reach a consensus stage. But so far, the only thing we've seen from you is "I don't see why I should change my approach because I it is just supposed to work" and "people should be mature enough to understand exactly what *I* want from them - why should I even have to bother telling them?" Oh, and one thing that I've stated over and over before, but which you keep blatantly ignoring, is that it would really help if you provided us with details of the trac issues you see critical for each release, because this is not something we can figure out unless we somehow level-up our mindreading skills. Someone's idea of critical, showstopper or must have for next release may be very different from yours... which is precisely why making sure that everybody is on the same page, and that we get a clearer idea of what you actually see as roadblocks and what you want to prioritize, through active communication, goes a long way. Call it "babysitting" if you will, but I don't think keeping a project functional lets maintainers escape some form of "babysitting". > If someone cares about a change then they have to > take action and contribute actively. You'll find that this is what most people who submit patches do. They care enough about a change to submit a patch... which ultimately ends up not being integrated for months at a time. How about Hans' getspeed patch then? As far as I recall we got most of the pieces in a (relatively) reasonable time, the patch was formally discussed and, I would assert, reviewed by all (but not explicitly acked, because, at least as far as I'm concerned, I didn't see the need to explicitly do so after it was discussed and consensus was reached)... Clearly we filled the "someone(s) cared enough about a change and taking action" step. Yet its integration still took months. Same goes for strerror, where we have some fairly solid consensus on how we should reinstate it in our code. And since you were the one who removed it, and we are assuming that you also followed the discussion (after all, you did care enough about strerror to remove it), we will assume that you are going to be the one to produce a patch for re-integration. Yet, still no sterror back... Therefore, you'll excuse me when I state that your "people have to take action and contribute actively" doesn't carry much weight to try to prove your point. Especially if, in line with what you indicate above, I have to assume that "taking action" for you means "formally acking" a patch that was submitted to the list or trac, and ensuring that you do not have any work left to do whatsoever during integration. > If they don't take action I can > only assume that they don't care too much about that change. (Either > not reviewing it, or not bothering to feed back after review.) So, if there's only one person feeling strongly about a change (eg. fixing MSVC6, or anything that has to do with Windows for that matter) and most of our established contributors, who don't use that platform, don't care as long as it doesn't interfere with the code they use, the change is doomed from the start, because we'll be waiting on an 'ack' that is very unlikely to come. I guess that explains why the Windows integration took 2 years and I had to resubmit the patchset 6 or 7 times. Curiously however, I don't feel like blaming Orin, Michael, Graeme, Tim and everybody who took some interest in the Windows backend patches, yet didn't formally ack all the ones that fell through and that I had to resubmit over and over again... Do you still not see a problem that should be addressed here? IIRC, Hans got tired of waiting for you to integrate the patch that everybody else was OK with, and had to create his own libusb-1.0 distro branch to compensate. So, no, unlike what you state our main issue is not that others are not taking action. Only you. > When I can contribute time then I review patches myself, Yes, you having the time to do anything useful for libusb (and not leting anybody take some of that workload from you) is one, if not _the_, major issue. As is clear from our discussions, you are also very unwilling to take any positive step to try to address it. > rather than > tell someone else to do it. I expect everyone else to do the same. I > don't know how many times I have said that anyone and everyone is > invited to do review, This "laissez faire" approach clearly doesn't work, because, even when things get reviewed and acked, they do not get integrated. We have a problem with an increasing backlog, and I am suggesting the one course of action that I have seen used, to great effect, both professionally as well as in other OSS projects. Please don't act surprised that people are not throwing themselves at trying to push the opened trac issues forward, when the first thing they see is that our maintainer isn't integrating reviewed patches in a timely manner. > and it's getting old. Go for it. What is getting old is that when we do review patches and reach a consensus, as highlighted in the 2 examples above, but we don't see anything happening from you, yet, somehow, this dysfunction is not something you need to act on. But to come back to the point, sorry but no, it is a maintainer's job to rally the troops when things are not progressing well. A maintainer that is unable to understand this simple concept strikes me as a poor choice for an OSS project. Regards, /Pete |
From: Ludovic R. <lud...@gm...> - 2011-12-13 07:57:04
|
2011/12/12 Sean McBride <se...@ro...>: > On Fri, 9 Dec 2011 21:32:29 -0500, Michael Plante said: > >>>> -void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level level, >>>> +static void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level >>level, >>>> const char *function, const char *format, va_list args) >>>> { >>>> FILE *stream = stdout; >>>> -- >>>> 1.7.7 >> >> >>This function is used in files other than core.c. > > Quite right. OK, so how to fix this warning: > > core.c:1683:6: warning: no previous prototype for function 'usbi_log_v' [-Wmissing-prototypes] > > Declare a prototype in core.c? in libusbi.h? Conditionally tag it static depending on OS? I proposed a fix in ticket #122 [1] 3 months ago. The issue and possible fix has already been discussed in June 2010 on this mailing list. The funny aspect of the libusb project is that the same bug is discussed again and again just because the proposed patch is not applied and a fixed version is not released. I bet we will still discuss about this warning in one year. Maybe it is time to take the power by the force. And replace the benevolent dictator [2]? Bye, [1] http://libusb.org/ticket/122 [2] http://en.wikipedia.org/wiki/Benevolent_Dictator_For_Life -- Dr. Ludovic Rousseau |
From: Tormod V. <lis...@gm...> - 2011-12-13 08:51:26
|
On Tue, Dec 13, 2011 at 8:56 AM, Ludovic Rousseau <lud...@gm...> wrote: > 2011/12/12 Sean McBride <se...@ro...>: >> On Fri, 9 Dec 2011 21:32:29 -0500, Michael Plante said: >> >>>>> -void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level level, >>>>> +static void usbi_log_v(struct libusb_context *ctx, enum usbi_log_level >>>level, >>>>> const char *function, const char *format, va_list args) >>>>> { >>>>> FILE *stream = stdout; >>>>> -- >>>>> 1.7.7 >>> >>> >>>This function is used in files other than core.c. >> >> Quite right. OK, so how to fix this warning: >> >> core.c:1683:6: warning: no previous prototype for function 'usbi_log_v' [-Wmissing-prototypes] >> >> Declare a prototype in core.c? in libusbi.h? Conditionally tag it static depending on OS? > > I proposed a fix in ticket #122 [1] 3 months ago. The issue and > possible fix has already been discussed in June 2010 on this mailing > list. > > The funny aspect of the libusb project is that the same bug is > discussed again and again just because the proposed patch is not > applied and a fixed version is not released. I bet we will still > discuss about this warning in one year. Maybe it would help if those who review the patch add their acknowledgement (Tested-by/Reviewed-by/etc) to the Trac ticket? From simply looking at the ticket it is not clear if the patch has been tested by anyone else than its author. But I do not know if you always use Trac for submitting patches. If patches normally go through the mailing list only, I would suggest adding [PATCH] to the mail subject, and follow-up e-mails with clear acknowledgements, as is customary in many other projects. This makes it much easier for the maintainer (maintainers, ideally) to apply patches without having to double-check everything themselves. Actually it makes it so easy that there would be no excuses. Regards, Tormod |
From: Peter S. <pe...@st...> - 2011-12-13 09:42:13
|
Ludovic Rousseau wrote: > > OK, so how to fix this warning: > > > > core.c:1683:6: warning: no previous prototype for function 'usbi_log_v' [-Wmissing-prototypes] > > > > Declare a prototype in core.c? in libusbi.h? Conditionally tag it static depending on OS? > > I proposed a fix in ticket #122 [1] 3 months ago. The issue and > possible fix has already been discussed in June 2010 on this > mailing list. Unfortunately Sean and Pete didn't notice the ticket and/or recall the discussion. This will happen again, to me or you or someone else. > The funny aspect of the libusb project is that the same bug is > discussed again and again just because the proposed patch is not > applied and a fixed version is not released. Or because different contributors use different processes for problems, and thus overlook documented solutions by others. I would argue that releases don't matter at all for coordination, since contributors need to synchronize with much higher granularity than that anyway; that's why we have the mailing list, Trac tickets and IRC. In Sean's defense I'm not sure he was on the list when the issue was discussed before. On the other hand it would be nice if he had looked at open tickets to avoid duplicating effort. Maybe he solved the issue around the same time as you did, and he also found a problem with creating a ticket for it, but unlike you didn't mention it on the list, and so there was duplicated effort. On the other hand maybe that's not such a big deal for this particular ticket. > I bet we will still discuss about this warning in one year. I'll take that bet, but I we may still discuss the tools we use for coordination. Since I kinda like Trac I also work on making it perform better for us. > Maybe it is time to take the power by the force. I don't think you need to apply very much force for this patch. The only question I have is if stdarg.h and va_list will be available in all the Windows build environments, so that this patch will not break anything which worked before. But Michael is the MSVC6 guy and it looks like he supports it, so I've committed it. I was planning on releasing 1.0.9 with this ticket open since I kindasorta didn't want to add things after the rcs. But there were a few other things as well, so let's see, it can only break, and then we can fix it easily. //Peter |
From: Pete B. <pe...@ak...> - 2011-12-13 11:56:30
|
On 2011.12.13 09:41, Peter Stuge wrote: > Ludovic Rousseau wrote: >> I proposed a fix in ticket #122 [1] 3 months ago. The issue and >> possible fix has already been discussed in June 2010 on this >> mailing list. > > Unfortunately Sean and Pete didn't notice the ticket and/or recall > the discussion. This will happen again, to me or you or someone else. And this is especially true when, on pretty much any other project, people wouldn't have to remember that this was discussed months ago, as, in the absence of any objection, an elementary change as this one would have been applied to the maintainer into the mainline repo in a matter of weeks. For the record, stdargs.h is a pretty standard header and has been part of MSVC and the WDK for some time, so I wouldn't personally see this patch as one that requires much scrutiny before integration. And indeed, this kind of short-term memory issue will happen again and again as long as *some people* still cannot accept that delaying the integration of simple patches for months is hugely detrimental to the project, and is the principal cause of libusb being as unhealthy as it is today. > I would argue that releases don't matter at all for coordination, > since contributors need to synchronize with much higher granularity > than that anyway; Well, since releases that include a specific patch are meant to happen after contributors have reviewed that patch, it's hard to argue with that. In case this isn't obvious, none of us here is advocating including patches into a release, in order to get it reviewed. If this is still your conception of what RERO stands for, and why you don't want to do anything that may be remotely construed as RERO, we have a problem... > that's why we have the mailing list, Trac tickets and IRC. I'll speak for myself here in not seeing IRC as something that should be part of the review process for integration (which may be another issue we're having with your release process - have you just been waiting for months to see if someone would manifest themselves on IRC wrt the RCs?). There's hardly any track record for IRC, as opposed to trac or e-mail, it requires immediacy (if you're not there when someone reports and issue, tough luck) and also, I'm not aware of anybody else but you using it at the moment. I do very much see its use for flashrom, where, if you get a bad flash, you want immediate assistance. For libusb, not so much. Nice to have if there are some people willing to monitor it, but I don't think it should be listed in the same breadth as trac & the mailing list. > I'll take that bet, but I we may still discuss the tools we use for > coordination. Since I kinda like Trac I also work on making it > perform better for us. A better performing trac would indeed help, as its sluggishness made it a real annoyance to use. Today, it seems to perform pretty well, along with the site. If you tweaked something, good job! If I have a chance in the coming weeks, I'll try to play with trac, but I can't promise anything. But can you at least provide a copy of the trac.ini used on libusb.org? Surely, you must have tweaked that file a little bit, so, while the trac code may be unmodified, the ini probably isn't. It would also help to check that the notification section is properly set up ([1] or [2]). Are the always_notify_* set as they should? Also, logs do help tremendously on an issue that should be very easy to reproduce on the server. It's fine and all to ask people to attempt to reproduce the issue by saying "yeah, I haven't really modified anything, so you should see it too", when, if trac can produce logs, you should get an explicit "Trac speaking here: FYI, I wasn't able to send a mail notification when you closed that issue because of XYZ", which is both the 1st item we should attempt to check here and the easiest way to identify the issue. How do we know it's not a problem with SMTP access on your side? What concerns me with your approach is that you are basically asking people to go spend time IN CASE the issue is not tied to your environment or configuration. That's a pretty big IF if you ask me, and hardly what I'd call effective troubleshooting. Effective troubleshooting would start at gathering all the relevant information from your server(s), since it should be possible to get insightful logs there, and leave blind replication attempts to the next step, if clues are unavailable on the server. Also, I doubt blind replication attempts will help much for the libusb.org unreachable issue, especially as there's a reverse proxy. For starters, have you tried configuring libusb.org not to go through nginx so that we can eliminate it as a potential problem? In my view, what you are asking right now is for others to spend a lot more time than what you would actually need trying to gather relevant data from your server's logs, just so you can be spared that task (and again, I don't have a problem looking at your logs for you if you don't want to do it yourself, provided I have access to the server to conduct tests). So please don't act too surprised if help isn't forthcoming... >> Maybe it is time to take the power by the force. > > I don't think you need to apply very much force for this patch. > > The only question I have is if stdarg.h and va_list will be available > in all the Windows build environments, so that this patch will not > break anything which worked before. But Michael is the MSVC6 guy and > it looks like he supports it, so I've committed it. > > I was planning on releasing 1.0.9 with this ticket open since I > kindasorta didn't want to add things after the rcs. But there were a > few other things as well, so let's see, it can only break, and then > we can fix it easily. For the record, the sooner changes are integrated to mainline, the easier it is to get reports from contributors as, even if we don't specifically test for that specific patch. On a healthy project, here's what would have occurred with Ludovic's patch: 1. Patch is submitted to trac and possibly discussed 2. Once the discussion dies down, and no objection has been raised, or if no discussion really occurred, it is applied to mainline by one of 2 or 3 maintainers of the project. As is the case on pretty much any other OSS project out there, this should occur in a matter of weeks, not months. 3. As mainline is a frequently updated repo, contributors follow it for their "derivative" work, and therefore potential leftover issues introduced by the patch, if any, are quickly identified. Oh, and please keep the list posted on when you plan to release, so that we have a chance to test our platforms before the actual release happens. I'd hate to see a last minute small fix being added to mainline causing problems. Regards, /Pete [1] https://sourceforge.net/apps/trac/ganglia/wiki/TracIni [2] http://trac.edgewall.org/wiki/TracNotification |
From: Pete B. <pe...@ak...> - 2011-12-13 19:29:54
|
On 2011.12.13 18:36, Segher Boessenkool wrote: >> A patch for mainline was sent less than a week ago (2011.12.07) and >> reviewed/acked by Segher. > > I did no such thing. I very much think you did, but you haven't realized it just yet ;) You did review part of the patch, since you pointed to stuff you wanted changed. To me that's about as good as ack/review we can get on this patch (see below). Also the slash in "reviewed/acked" means pick whichever semantically pleases you, rather than an "and". So we can start debating the semantics of partial review equating review or review meaning thorough review if you want... But, let's consider the implications of you saying the patch has not actually been reviewed/acked. Logically, this means that someone else is supposed to "review" the MSVC6 part then, which, considering that the original poster (Satz) submitted most of the patch, which was left untouched, and that I added a bit more to fix additional issues I picked, pretty much means that this reviewed would have to be someone who isn't myself or Satz. So who's next to review it? The only people we have that I know are still using MSVC6 and would comment are Michael or Graeme. I don't think Graeme is interested in participating in libusb-1.0 that much these days (can't blame him) and Michael also participates on an on/off basis, so we might be in for the long wait. Moreover, Michael did get involved in other aspects of the MSVC6 thread, which tells me that either he has skimmed over the patch and not found anything obvious, or has no interest in reviewing it and somebody else will have to fit that role. Thus, if we agree that you did not "review/ack" the patch, and since we don't have anybody in line that is susceptible to formally review the MSV6 part, we have to wait for a formal review/ack by someone else than the persons listed above, and someone who is also obviously not our maintainer either, since he indicated that he expects other people to have conducted review before integration. So what are we supposed to do now? State that the patch wasn't reviewed/acked and wait months for some benevolent MSVC6 user to dig an old thread and indicate that they are OK with the patch? Or, we consider that a review of impact of the patch on core, which is exactly what you did, is about as good a formal review as we're gonna get on this patch, and therefore can consider the patch as "acked/reviewed". Regards, /Pete |
From: Michael P. <mic...@gm...> - 2011-12-14 03:40:36
|
Pete Batard wrote: >> I don't think Graeme is interested in participating in libusb-1.0 that >> much these days (can't blame him) and Michael also participates on an >> on/off basis, so we might be in for the long wait. In progress. Like you said, it may be on/off. My current state will be pushed to windows_merge5 (rewritten as a policy), and will be based on the libusb.git master (which I may occasionally refer to as Daniel's, because I find that less confusing). I'm going to start digging through your repo soon, but not tonight. Here are some things I'm finding when I rebase a few of my old commits onto daniel/master. 1) I receive the following when I try to build OR clean the dll project: libusb\libusb-1.0.rc(21): Could not find the file LIBUSB_RC. ...seems to be a warning, as it still compiles and the version still winds up in the dlls. Not sure what's up there. 2) guid_to_string -- I need to look into this, but it seems to be used but its definition is #ifdef-ed out. If you want to point me at the right commit in your repo, that'd be helpful, but I'll find it eventually, given enough time... Thanks, Michael |
From: Pete B. <pe...@ak...> - 2011-12-14 13:51:47
|
On 2011.12.14 03:40, Michael Plante wrote: > 1) I receive the following when I try to build OR clean the dll project: > > libusb\libusb-1.0.rc(21): Could not find the file LIBUSB_RC. I'll try to keep an eye out for it next time I test on MSVC6. Line #21 is just the version, and there's no mention of a LIBUSB_RC variable in the RC so not sure what's going on with the resource compiler here either. > 2) guid_to_string -- I need to look into this, but it seems to be used but > its definition is #ifdef-ed out. Yeah, because, unless I overlooked something for MSVC6, it's only used for logging right now (windows_usb.c #157) > If you want to point me at the right > commit in your repo, that'd be helpful, but I'll find it eventually, given > enough time... Or you should use a git GUI that provides git blame (I seriously have no idea how people can use git blame outside of an UI - for those interested, here's a screenshot of what you get in Tgit: http://img443.imageshack.us/img443/3702/gitblame.png). The commit for the #ifdef addon is http://git.libusb.org/?p=libusb-pbatard.git;a=commit;h=11c6f4d4ce75092ef21fd6a961e7dd75d38629ee;js=1 Regards, /Pete |
From: Michael P. <mic...@gm...> - 2011-12-14 14:05:13
|
Pete: this is incomplete. I'm just letting you know where I am, since there was some hint of impatience. (I have no comments below at this time, thus the top-post.) -----Original Message----- From: Pete Batard [mailto:pe...@ak...] Sent: Wednesday, December 14, 2011 8:52 AM To: lib...@li... Subject: Re: [Libusb-devel] [PATCHES] thread safety on darwin andminorotherchanges On 2011.12.14 03:40, Michael Plante wrote: > 1) I receive the following when I try to build OR clean the dll project: > > libusb\libusb-1.0.rc(21): Could not find the file LIBUSB_RC. I'll try to keep an eye out for it next time I test on MSVC6. Line #21 is just the version, and there's no mention of a LIBUSB_RC variable in the RC so not sure what's going on with the resource compiler here either. > 2) guid_to_string -- I need to look into this, but it seems to be used but > its definition is #ifdef-ed out. Yeah, because, unless I overlooked something for MSVC6, it's only used for logging right now (windows_usb.c #157) > If you want to point me at the right > commit in your repo, that'd be helpful, but I'll find it eventually, given > enough time... Or you should use a git GUI that provides git blame (I seriously have no idea how people can use git blame outside of an UI - for those interested, here's a screenshot of what you get in Tgit: http://img443.imageshack.us/img443/3702/gitblame.png). The commit for the #ifdef addon is http://git.libusb.org/?p=libusb-pbatard.git;a=commit;h=11c6f4d4ce75092ef21fd 6a961e7dd75d38629ee;js=1 Regards, /Pete |
From: Michael P. <mic...@gm...> - 2011-12-22 01:17:58
|
On Wed, Dec 14, 2011 at 7:51 AM, Pete Batard <pe...@ak...> wrote: > > On 2011.12.14 03:40, Michael Plante wrote: > > 1) I receive the following when I try to build OR clean the dll project: > > > > libusb\libusb-1.0.rc(21): Could not find the file LIBUSB_RC. > > I'll try to keep an eye out for it next time I test on MSVC6. > > Line #21 is just the version, and there's no mention of a LIBUSB_RC > variable in the RC so not sure what's going on with the resource > compiler here either. Not in yours, but there is in Peter's. > > > 2) guid_to_string -- I need to look into this, but it seems to be used but > > its definition is #ifdef-ed out. > > Yeah, because, unless I overlooked something for MSVC6, it's only used > for logging right now (windows_usb.c #157) If you look at the GEN_PASS case in windows_get_device_list, you'll see it's used unconditionally. However, the definition is conditional. It turns out I had a slightly different version of msvc/config.h (related to toggleable logging, go figure), so I ran into this. if (s == ERROR_SUCCESS) { if (nb_guids >= MAX_ENUM_GUIDS) { // If this assert is ever reported, grow a GUID table dynamically usbi_err(ctx, "program assertion failed: too many GUIDs"); LOOP_BREAK(LIBUSB_ERROR_OVERFLOW); } if_guid = (GUID*) calloc(1, sizeof(GUID)); pCLSIDFromString(guid_string_w, if_guid); guid[nb_guids++] = if_guid; usbi_dbg("extra GUID: %s", guid_to_string(if_guid)); } > > If you want to point me at the right > > commit in your repo, that'd be helpful, but I'll find it eventually, given > > enough time... > > Or you should use a git GUI that provides git blame (I seriously have no > idea how people can use git blame outside of an UI - for those > interested, here's a screenshot of what you get in Tgit: > http://img443.imageshack.us/img443/3702/gitblame.png). The commit for > the #ifdef addon is > http://git.libusb.org/?p=libusb-pbatard.git;a=commit;h=11c6f4d4ce75092ef21fd6a961e7dd75d38629ee;js=1 That commit can only make things worse, not better, as described above. I'll have more to say when I finish cleaning up my repo. But have you had a chance to pick through the diffs against official recently? (Also, I probably won't be reading every email in here unless it's obviously for me until early January, as I'm stuck with the gmail web interface until then.) Thanks, Michael |
From: Pete B. <pe...@ak...> - 2011-12-22 14:02:03
|
On 2011.12.22 01:17, Michael Plante wrote: >> Line #21 is just the version, and there's no mention of a LIBUSB_RC >> variable in the RC so not sure what's going on with the resource >> compiler here either. > > Not in yours, but there is in Peter's. Ah, you're using Peter's. Now it makes sense. I thought you were using -pbatard, so I really didn't get quite what your issues were (not that I have really tested -pbatard against MSVC6 lately). >>> 2) guid_to_string -- I need to look into this, but it seems to be used but >>> its definition is #ifdef-ed out. >> >> Yeah, because, unless I overlooked something for MSVC6, it's only used >> for logging right now (windows_usb.c #157) > > If you look at the GEN_PASS case in windows_get_device_list, you'll > see it's used unconditionally. However, the definition is > conditional. Not exactly. The conditional part applies to logging being on or not and in GEN pass, we have usbi_dbg(..., guid_to_string(...)) with usbi_dbg() only being defined #if defined(ENABLE_DEBUG_LOGGING) || defined(INCLUDE_DEBUG_LOGGING) which is the same condition as guid_to_string() having a body. The only problem is that on MSVC6, you define usbi_dbg as an variadic inline with an empty body, therefore the compiler still attempts to resolve the arguments, whereas for all the other compilers, it is defined as a variadic macro, whose arguments the compiler will be happily ignored and which is what we want. So it looks like we need to add a new #ifdef that ignores the logging #ifdefs around guid_to_string() for the sake of MSVC6, and we're likely to run into similar issues if anybody who modifies the code adds some debug-only calls and doesn't realize that MSVC6 doesn't use macros for logging... >>> If you want to point me at the right >>> commit in your repo, that'd be helpful, but I'll find it eventually, given >>> enough time... >> >> Or you should use a git GUI that provides git blame (I seriously have no >> idea how people can use git blame outside of an UI - for those >> interested, here's a screenshot of what you get in Tgit: >> http://img443.imageshack.us/img443/3702/gitblame.png). The commit for >> the #ifdef addon is >> http://git.libusb.org/?p=libusb-pbatard.git;a=commit;h=11c6f4d4ce75092ef21fd6a961e7dd75d38629ee;js=1 > > That commit can only make things worse, not better, as described above. The commit had already been applied in both Peter and mine. Should I understand that you were looking for a commit which you thought I had only applied in mine but not in Peter's to address the issue? Or am I still missing the point of why you are looking for the commits pertaining to guid_to_string()? > I'll have more to say when I finish cleaning up my repo. But have you > had a chance to pick through the diffs against official recently? I didn't because I had no clue what had changed from official (or whether anything had actually changed) and because supposedly a release is just around the corner (or another RC) so it's not a smart move to process anything until then. If there's one thing these 2 years trying to develop for libusb have taught me, it is that trying to follow libusb as you would any other project (through rebases, etc) or having any sense of urgency, is a waste of time - you're better off following the example of our great chairman and just wait... Regards, /Pete |
From: Peter S. <pe...@st...> - 2011-12-14 04:03:49
|
Pete Batard wrote: > we can start debating the semantics of partial review equating > review or review meaning thorough review if you want... Review means someone has looked at the patch. Partial review means someone has only looked at parts of the patch. All review leads to feedback about that which was reviewed, either that it is good or that it needs further work. If all of the patch gets the right feedback then I will need to spend much less time looking at it myself, and thus it ends up in libusb.git master faster. > So who's next to review it? The only people we have that I know are > still using MSVC6 and would comment are Michael or Graeme. If noone cares enough then it does not get committed. Simple as that. Of course the author cares, and I also care, but I think you understand that patches take longer to reach libusb.git master when fewer people are doing review, and this is a really important point. Thanks! > someone who is also obviously not our maintainer either, since he > indicated that he expects other people to have conducted review > before integration. No, I didn't indicate that. What I have written, over and over, is that I am not the only one who is allowed or supposed to do review, in fact quite the opposite; *everyone* is allowed and supposed to do review, all according to the time they can contribute of course. It's an ideal way to contribute, learn a codebase, and ultimately to become an even more efficient and valuable contributor in the future. This works the same in pretty much every open source project; I'm sure you recognize the pattern. //Peter |
From: Michael P. <mic...@gm...> - 2011-12-13 14:13:18
|
Peter Stuge wrote: >> But Michael is the MSVC6 guy and >> it looks like he supports it, so I've committed it. By inspection, I don't see any reason it would break my build, but, that said, I stopped trying to follow along in git ever since our last CRLF discussion about a year ago, since git kept misbehaving. I may need to pick it up again, as libusb.git master seems to have a lot of issues right now. The testing branch also doesn't appear to have this commit, unless I missed it. libusb-pbatard compiles just fine, fwiw. I'll have to try to look at this in the next couple days, I guess. Thanks, Michael |
From: Sean M. <se...@ro...> - 2011-12-13 16:16:18
|
On Tue, 13 Dec 2011 10:41:57 +0100, Peter Stuge said: >> > OK, so how to fix this warning: >> > >> > core.c:1683:6: warning: no previous prototype for function >'usbi_log_v' [-Wmissing-prototypes] >> > >> > Declare a prototype in core.c? in libusbi.h? Conditionally tag it >static depending on OS? >> >> I proposed a fix in ticket #122 [1] 3 months ago. The issue and >> possible fix has already been discussed in June 2010 on this >> mailing list. > >Unfortunately Sean and Pete didn't notice the ticket and/or recall >the discussion. This will happen again, to me or you or someone else. Good grief... I have never seen such lengthy discussions over such trivial fixes for mere warnings. :( For the record, I never thought to check Trac for a months old bug to add a mere function prototype! (Though I probably should have, since this is libusb!) :( I'd like to second the suggestion of using gerrit <http://code.google.com/p/gerrit/>. I think it would help this project's currently dysfunctional workflow. Cheers, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Peter S. <pe...@st...> - 2011-12-13 12:09:41
|
Pete Batard wrote: > On a healthy project, here's what would have occurred with Ludovic's patch: > > 1. Patch is submitted to trac and possibly discussed I think that if the patch had been in Trac before the rcs instead of just after, then it would have been included already in those rcs and not as late as today. Agreed that Trac trouble has caused problems. Yes, I continue to spend time on Trac to make it work better for us. > Oh, and please keep the list posted on when you plan to release, so > that we have a chance to test our platforms before the actual > release happens. Actually no, libusb.git master needs to be releaseable at all times. So that testing should happen between patch being published and when there is agreement to add it to libusb.git master, so to say. > I'd hate to see a last minute small fix being added to mainline > causing problems. Yes and no. We can fix it if there's a problem. //Peter |