[libvirt] Consultation of the patch "util: Prepare helpers for unpriv_sgio setting"

[yuxh]

On 03/13/2013 03:52 PM, Osier Yang wrote:
> On 2013年03月13日 15:38, Osier Yang wrote:
>> On 2013年03月13日 15:20, yuxh wrote:
>>> On 03/13/2013 11:33 AM, Osier Yang wrote:
>>>> On 2013年03月13日 11:04, yuxh wrote:
>>>>> To Osier Yang
>>>>>
>>>>> Sorry for disturbing you on the working time, but I really need your
>>>>> help.
>>>>> I met a problem when I do the virtualization test on the 
>>>>> environment of
>>>>> (Host: RHEL6.4GA. Guest: RHEL6.4GA) and it seems to have relationship
>>>>> with your patch of "util: Prepare helpers for unpriv_sgio setting".
>>>>
>>>> Never mind, it's the bug I produced. :(
>>>>
>>>>>
>>>>> I added a disk to virtio-scsi bus through the VM's XML file like 
>>>>> this:
>>>>> <disk type='block' device='disk'>
>>>>> <driver name='qemu' type='raw' io='threads'/>
>>>>> <source dev='/mnt/test.raw'/>
>>>>> <target dev='sdb' bus='scsi'/>
>>>>> <shareable/>
>>>>> </disk>
>>>>>
>>>>> Then when I started the VM, I met the error.
>>>>> [root at build SOURCES]# virsh start pan64ga
>>>>> error: Failed to start domain pan64ga
>>>>> error: Unable to get minor number of device '/mnt/test.raw': Invalid
>>>>> argument
>>>>>
>>>>> [root at build SOURCES]#
>>>>>
>>>>> I tried other source files and other way of using if and I found:
>>>>> 1). If we use the files which format is raw, qed and qcow2 as the 
>>>>> disk
>>>>> this error occur again.
>>>>> 2). If we use partition(such as /dev/sdb1) or whole disk(such as
>>>>> /dev/sdb) this error doesn't occur.
>>>>> 3). If we delete the "<shareable/>" then files, partition and disk 
>>>>> all
>>>>> can be used correctly.
>>>>>
>>>>> And I investigate the error msg. it is returned by 
>>>>> qemuGetSharedDiskKey
>>>>> fuction.
>>>>> char *
>>>>> qemuGetSharedDiskKey(const char *disk_path)
>>>>> {
>>>>> ...
>>>>> // error occur here. return value is -22
>>>>> if ((rc = virGetDeviceID(disk_path, &maj, &min)) < 0) {
>>>>> virReportSystemError(-rc,
>>>>> _("Unable to get minor number of device '%s'"),
>>>>> disk_path);
>>>>> return NULL;
>>>>> }
>>>>> ...
>>>>> }
>>>>>
>>>>> Go on the virGetDeviceID fuction:
>>>>> int
>>>>> virGetDeviceID(const char *path, int *maj, int *min)
>>>>> {
>>>>> ...
>>>>>
>>>>> if (!S_ISBLK(sb.st_mode)) {
>>>>> VIR_FREE(canonical_path);
>>>>> return -EINVAL; // error occur here and it return -22
>>>>> }
>>>>> ...
>>>>> }
>>>>>
>>>>> So the error occur place is "S_ISBLK(sb.st_mode)" and I tried 
>>>>> compiled
>>>>> the libvirt with these lines deleted and the error didn't happen 
>>>>> again.
>>>>> Also I checked the disk if VM and it worked well.
>>>>> I found this fuction virGetDeviceID is come from your patch of "util:
>>>>> Prepare helpers for unpriv_sgio setting". But I am not familiar with
>>>>> libvirt.
>>>>> So I have to consult with you about whether this "S_ISBLK" check is
>>>>> vital and If we can delete it or
>>>>> find out some method to avoid this error so we can use the file as
>>>>> shared disk in VM.
>>>>
>>>> It's fixd by
>>>> https://www.redhat.com/archives/libvir-list/2013-February/msg01024.html, 
>>>>
>>>> which is already committed to upstream, and backported
>>>> to 6.4.z. You will see the fix in the new build, which will come soon.
>>>>
>>>> Regards,
>>>> Osier
>>> Sorry, I have tried the the libvirt's upstream code and it still have
>>> this error.
>>> According to the code, if we use "<disk type='block' device='disk'> "
>>> and "<shareable/> " to set the disk's configuration in VM's XML 
>>> file, it
>>> will go through like below:
>>>
>>> int
>>> qemuAddSharedDisk(virQEMUDriverPtr driver,
>>> virDomainDiskDefPtr disk,
>>> const char *name)
>>> { ...
>>> if (!(key = qemuGetSharedDiskKey(disk->src)))
>>> goto cleanup;
>>> ...
>>> }
>>>
>>> char *
>>> qemuGetSharedDiskKey(const char *disk_path)
>>> {
>>> ...
>>> if ((rc = virGetDeviceID(disk_path, &maj, &min)) < 0) {
>>> virReportSystemError(-rc,
>>> _("Unable to get minor number of device '%s'"),
>>> disk_path);
>>> return NULL;
>>> }
>>> ...
>>> }
>>>
>>> int
>>> virGetDeviceID(const char *path, int *maj, int *min)
>>> {
>>> struct stat sb;
>>>
>>> if (stat(path, &sb) < 0)
>>> return -errno;
>>>
>>> if (!S_ISBLK(sb.st_mode))
>>> return -EINVAL;
>>>
>>> if (maj)
>>> *maj = major(sb.st_rdev);
>>> if (min)
>>> *min = minor(sb.st_rdev);
>>>
>>> return 0;
>>> }
>>>
>>> To make a long story short, the virGetDeviceID fuction want to get the
>>> maj and min number of the source file which we provided as the VM's
>>> disk. But the file such as /mnt/test.raw doesn't have the maj and min
>>> number.
>>> So virGetDeviceID return " -EINVAL" and qemuGetSharedDiskKey print the
>>> error message of "Unable to get minor number of device /mnt/test.raw".
>>> That's the reason the VM can't be started.
>>> So I just want to know now, if the way I used a file to act as VM's 
>>> disk
>>> and set "<disk type='block' device='disk'>" , "<shareable/> " at the
>>> same time is reasonable.
>>
>> Hum, sorry I didn't read your problem carefully. But...
>>
>> It's not reasonable. As the disk source you use is /mnt/test.raw, I
>> believe it's not a block device. However, you used disk type='block'.
>> Libvirt honors what you specified, and tries to grab the device IDs
>> of the disk source (it expects the disk source is block, but it's not).
>
> This inspires me thinking of we should validate the disk source type
> somewhere (when parsing or in the driver, as it's general for all
> drivers).
>
Yes. That should be essential.
Thank you for your help.

yuxh
>>
>> Regards,
>> Osier
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
> -- 
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list