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

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@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@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).

> > Regards, > Osier > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

>>>>> 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@build SOURCES]# virsh start pan64ga >>>>> error: Failed to start domain pan64ga >>>>> error: Unable to get minor number of device '/mnt/test.raw': Invalid >>>>> argument

>> 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). > Any opinions on should we validate whether the disk type and the specified source matches? Just realized that validating when parsing doesn't make sense, because the validation can imply the checking of whether the disk source existing or not, however, for a inactive domain, it's normal that its disk source may not existing. So the only way to do the validation is inside driver. But does it imply we will have regression if doing so? As yuxh said, the guest can actually be started successfully with block disk has a plain file source, not sure if it's expected result of qemu though.