Help me get a patch into Wine

[Charles Davis]

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