On Mon, Oct 12, 2009 at 2:18 PM, Charles Davis <[email protected]> wrote: > Keith Muir wrote: >> Charles Davis wrote: >>> Damn it, I keep forgetting to reply all. >>> >>> Charles Davis wrote: >>> >>>> Vitaliy Margolen wrote: >>>> >>>>> Charles Davis wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> My name is Charles Davis, but you all can call me "Chip". >>>>>> >>>>>> I'm new to Wine development, and frankly, I'm kinda scared because I've >>>>>> heard and read that getting patches into Wine is very difficult. In >>>>>> fact, every time I got ready to send this first patch, I chickened out >>>>>> and couldn't do it. So, I want someone to review this patch to see if >>>>>> there's anything wrong with it. >>>>>> >>>>>> >>>>> Thanks for writing the patch. >>>>> >>>>> Few things about your patch: >>>>> >>>>>> + * Copyright 2008, 2009 Charles Davis >>>>>> >>>>> You need to add substantial amount of code to or be working for a long >>>>> time >>>>> on this file to add yourself to the copyright note. As for your >>>>> contribution >>>>> to the project, all authors are tracked and AUTHORS file updated >>>>> automatically. >>>>> >>>> Oh. I see. But I have many more patches for this file. >>>> >>>>>> + switch(fmt->Format) >>>>>> + { >>>>>> + case IOCTL_CDROM_CURRENT_POSITION: >>>>>> + memset(&data->CurrentPosition, 0, >>>>>> sizeof(data->CurrentPosition)); >>>>>> + break; >>>>>> + case IOCTL_CDROM_MEDIA_CATALOG: >>>>>> + memset(&data->MediaCatalog, 0, sizeof(data->MediaCatalog)); >>>>>> + break; >>>>>> + case IOCTL_CDROM_TRACK_ISRC: >>>>>> + memset(&data->TrackIsrc, 0, sizeof(data->TrackIsrc)); >>>>>> + } >>>>>> + /* We need IOCDAudioControl for IOCTL_CDROM_CURRENT_POSITION */ >>>>>> + if (fmt->Format == IOCTL_CDROM_CURRENT_POSITION) >>>>>> + { >>>>>> + ERR("This version of Mac OS X does not support >>>>>> IOCDAudioControl\n"); >>>>>> + return STATUS_NOT_SUPPORTED; >>>>>> + } >>>>>> >>>>> This doesn't seems right. If it's not supported (aka error) you shouldn't >>>>> be >>>>> touching returned data. Also why are you zeroing return buffers? I don't >>>>> see >>>>> other implementations do it. >>>>> >>>> Good catch. I should have done the check first. >>>> >>>>>> + case IOCTL_CDROM_MEDIA_CATALOG: >>>>>> + memset(&ioc, 0, sizeof(ioc)); >>>>>> >>>>> and >>>>> >>>>>> + case IOCTL_CDROM_TRACK_ISRC: >>>>>> + memset(&ioc, 0, sizeof(ioc)); >>>>>> >>>>> You shouldn't need to zero buffer you pass to system ioctl. >>>>> >>>> Good point. >>>> >>>>>> + if((io = ioctl(fd, DKIOCCDREADMCN, &ioc.mcn)) == -1) break; >>>>>> + if((io = ioctl(fd, DKIOCCDREADISRC, &ioc.isrc)) == -1) break; >>>>>> >>>>> Please keep formatting consistent with the rest of the function/file. Here >>>>> you need to add space after "if". >>>>> >>>> You're right. I read about that, but that must have slipped by me. >>>> >>>>> Vitaliy. >>>>> >>>> Thanks for your input. >>>> >>>> Chip >>>> >>> >>> >>> >>> >>> >>> >> >> The Julliard rank is something worth explaining. Wine developers have >> long wondered exactly *HOW* Alexandre determines whether or not a >> specific patch gets committed. The process is something as follows: >> >> * Does the patch use C++ style comments? Yes: REJECT >> * Does the patch change coding style from the surrounding code? Yes: >> REJECT >> * Is the code obviously wrong in any way? Yes: REJECT >> * Does it take more than a few seconds to see that the code is >> correct? If today is a busy day: IGNORE >> * Is the author on the s**t list? Yes: REJECT >> * Are the planets aligned in just the right way and none of the >> above reject? Yes : ACCEPT >> >> And of course now we need some sort of measurement for inclusion in the >> crap list. For every 'good' patch set an author moves further off the >> list, for every bad patch the author's reputation is diminished and put >> further onto the list. Most Wine developers have come to realize that >> that the Julliard Rank is very similar to Google's PageRank algorithm: >> everybody who cares understands how it is 'supposed' to work but could >> never replicate it in a million years. >> > OK, I've sent a better version of my patch to wine-patches. Now we'll > see what AJ thinks of it.
Apparently he liked it, it was committed today http://source.winehq.org/git/wine.git/?a=commitdiff;h=c7992a8d260e0ce4073f85988c81b5c09a8c127f Well done, now get some more patches in ;-) -- -Austin
