Help me get a patch into Wine

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