Help me get a patch into Wine

[Charles Davis]

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 in advance.

Chip

---
 dlls/ntdll/cdrom.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

[Nikolay Sivov]

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.

Lie. First of all avoid looking at native dumps, disasms, decompiled 
output etc., if you don't
it's quite possible to be committed.
> 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.

You're on a right way. As for me I'm not very competent in device 
accessing things but could you be
more specific what this patch is supposed to do, provide bug links or 
something. Looks like
a Mac (or not?) support for some call...A description would be nice.
> Thanks in advance.
>
> Chip

[Vitaliy Margolen]

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.
> - 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.
> - 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.
> -
>    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".

Vitaliy.