Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Laurent Vivier <lvivier@redhat.com>]

On 29/06/2015 05:01, Programmingkid wrote:
> 
> On Jun 28, 2015, at 8:29 PM, Laurent Vivier wrote:
> 
>> Hi,
>>
>> On 29/06/2015 01:43, Programmingkid wrote:
>>>
>>> On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote:
>>>
>>>> On 25 June 2015 at 18:56, Programmingkid
>>>> <programmingkidx@gmail.com <mailto:programmingkidx@gmail.com>> wrote:
>>>>> Nice to hear from you again Laurent. The only way a solution in
>>>>> hdev_open() would work is if it could prevent
>>>>> find_image_format() from executing. Otherwise find_image_format()
>>>>> would just quit QEMU with an error.
>>>>
>>>> The question you should be asking is "what is Linux doing for raw
>>>> CDROM devices that is different, such that it works there but
>>>> doesn't work on OSX?".
>>>>
>>>> It would also be helpful to know which is the case that doesn't
>>>> work. Does QEMU fail in all cases, or only if the cdrom drive is
>>>> empty, or only if there's a disk in the drive?
>>>
>>> QEMU fails if the cdrom is specified "-cdrom /dev/cdrom", and there
>>> is no cd in the drive.
>>>
>>> QEMU also fails with a real cdrom in the drive.
>>>
>>>>
>>>> My initial suspicion is that we need OSX support in raw-posix.c for
>>>> handling the host CDROM specially -- note that Linux and FreeBSD
>>>> register a bdrv_host_cdrom with an is_inserted function.
>>>
>>> The is_inserted function wouldn't make a difference.
>>
>> In fact, if your patch fixes the problem, the is_inserted with no
>> cdrom should too:
>>
>> with your " strcmp("/dev/cdrom", filename) == 0 ", you force the
>> selection of bdrv_raw (which is what to do).
>>
>> without your patch, if "bdrv_is_inserted()" was implemented and no cdrom
>> in the drive " !bdrv_is_inserted(bs)  " should also select bdrv_raw.
>>
>> It appears also that bdrv_host_cdrom is not registered in
>> bdrv_file_init(). I think this is the missing part to have a host cdrom
>> support on MacOS X.
>>
>> Laurent
> 
> This patch is what I came up with using your idea. It uses the fact that
> the bdrv_is_inserted() function is called first from
> find_image_format(). The bdrv_is_inserted() function now points
> to cdrom_is_inserted(). This new function will return 0 the first time
> it is called, then 1 after that. So far it works.

Great.

> 
> ---
>  block/raw-posix.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a967464..2d35580 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -2324,6 +2324,23 @@ static int hdev_create(const char *filename,
> QemuOpts *opts,
>      return ret;
>  }
> 
>  
> 
> +#ifdef __APPLE__
> +
> +static int cdrom_is_inserted(BlockDriverState *bs)
> +{
> +    static int count = 0;
> +    int returnValue = 1;
> +    
> +    if(count == 0) {
> +        returnValue = 0; // get around find_image_format() issue
> +    }
> +    
> +    printf("count = %d for %s, returning %d\n", count, bs->filename,
> returnValue);
> +    count++;
> +    return returnValue;
> +}
> +#endif

So instead, read the size of the device, if it is 0, return false.

Something like for FreeBSD.

raw_getlength(bs), and

raw_getlength() should use an ioctl() to get the size.

I think you can use something like DKIOCGETBLOCKCOUNT.

http://www.opensource.apple.com/source/xnu/xnu-1456.1.26/bsd/sys/disk.h
https://developer.apple.com/library/mac/samplecode/CDROMSample/Listings/CDROMSample_CDROMSample_c.html

>  static BlockDriver bdrv_host_device = {
>      .format_name        = "host_device",
>      .protocol_name        = "host_device",
> @@ -2365,6 +2382,10 @@ static BlockDriver bdrv_host_device = {
>      .bdrv_ioctl         = hdev_ioctl,
>      .bdrv_aio_ioctl     = hdev_aio_ioctl,
>  #endif
> +
> +#ifdef __APPLE__
> +    .bdrv_is_inserted   = cdrom_is_inserted,
> +#endif
>  };

We need also cdrom_eject, and cdrom_lock_medium.

This example can help:

http://www.opensource.apple.com/source/DiskArbitration/DiskArbitration-156/disktool/disktool.c

Laurent