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

[Programmingkid]

On Jun 25, 2015, at 11:32 AM, Programmingkid wrote:

> 
> On Jun 25, 2015, at 11:14 AM, Programmingkid wrote:
> 
>> 
>> On Jun 25, 2015, at 2:53 AM, Markus Armbruster wrote:
>> 
>>> Programmingkid <address@hidden> writes:
>>> 
>>>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>>>> 
>>>>> 
>>>>> 
>>>>> On 06/23/2015 01:56 PM, Programmingkid wrote:
>>>>>> Fix real cdrom detection so that a real cdrom can actually be used.
>>>>>> 
>>>>>> signed-off-by: John Arbuckle <address@hidden
>>>>>> <mailto:address@hidden>>
>>>>>> 
>>>>>> This patch has been tested on Mac OS X host and guest. 
>>>>>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>>>>>> 
>>>>>> Note: I was able to view the files using OpenBIOS, but not on 
>>>>>> Mac OS X. The size of the disc is reported correctly but some
>>>>>> error happens that prevents it from mounting in Mac OS X. This
>>>>>> is probably another bug with QEMU.
>>>>>> 
>>>>>> ---
>>>>>> block.c |    3 ++-
>>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>> 
>>>>>> diff --git a/block.c b/block.c
>>>>>> index dd4f58d..75ccfad 100644
>>>>>> --- a/block.c
>>>>>> +++ b/block.c
>>>>>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>>>>>> const char *filename,
>>>>>>  int ret = 0;
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>  /* Return the raw BlockDriver * to scsi-generic devices or empty
>>>>>> drives */
>>>>>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>>>>>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>>>>>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>>>>>      *pdrv = &bdrv_raw;
>>>>>>      return ret;
>>>>>>  }
>>>>>> -- 
>>>>>> 1.7.5.4
>>>>>> 
>>>>> 
>>>>> So what's the issue that this patch attempts to fix and how did you
>>>>> determine that the fix was needed here? It doesn't look like it respects
>>>>> proper abstraction at a glance.
>>>> 
>>>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom"
>>>> option is given.
>>>> 
>>>> Before the patch, the bdrv_open_inherit() function would be
>>>> incorrectly called. Its documentation says "Opens a disk image (raw,
>>>> qcow2, vmdk, ...)" meaning only for disk image files (not for real
>>>> media). This patch prevents the bdrv_open_inherit() function from ever
>>>> being called. It sets the pdrv variable to the raw format. This made
>>>> sense to me since a real cdrom is read in the raw format.
>>>> 
>>>> A quick test does show the patch works. A real cdrom is successfully
>>>> opened on qemu-system-i386 using a Windows XP guest.
>>> 
>>> What about /dev/sr0, /dev/sr1, and whatever other names could refer to a
>>> block device without a medium?
>>> 
>>> Comparing filenames isn't a good way to test "is a block device without
>>> a medium".
>> 
>> I think we are going to have to agree to disagree. I have never used the 
>> /dev/sr(0 | 1) devices and don't see how they would be effected by this 
>> patch. Are you trying to say the /dev/sr(0 | 1) devices *should* be handled 
>> by this patch?
> 
> Thinking about your question some more, I see what you mean. On Linux 
> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom link refers 
> to the /dev/sr0 device file. So if you just use /dev/cdrom, you are good.

Just had another idea. I could change the patch so that if it detects any 
device file being used, it handles it as raw. That way if you want to use 
/dev/sr0, you can. 

 /* Return the raw BlockDriver * to scsi-generic devices or empty
drives */
-    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
+    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
+                      || strncmp("/dev/", filename, strlen("/dev/")) == 0) {
     *pdrv = &bdrv_raw;
     return ret;
 }