Re: [libvirt] [PATCH 4/5] vz: report disks either as disks or filesystems depending on original xml

On 09.12.2016 17:36, Maxim Nestratov wrote: > Virtuozzo SDK interface doesn't differ filesystems from disks and sees them as disks. > Before, we always mistakenly presented disks based on files as filesystems, which is > not completely correct. Now we are going to show either disks or filesystems depending > on a hint, which uses boot device section of VZ config. Though this information > doesn't change booting order of a CT, it is used by vz libvirt interface as a hint > for libvirt representation of disks. Since now, if we have filesystems in input xml, > then we add them to VZ booting devices list and rely on this information to show > corresponding libvirt xml. > > Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; > --- > src/vz/vz_sdk.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 86 insertions(+), 10 deletions(-) > > diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c > index 1030a1b..9b8b48e 100644 > --- a/src/vz/vz_sdk.c > +++ b/src/vz/vz_sdk.c > @@ -50,6 +50,9 @@ static PRL_HANDLE > prlsdkFindNetByMAC(PRL_HANDLE sdkdom, virMacAddrPtr mac); > static PRL_HANDLE > prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk); > +static bool > +prlsdkInBootList(PRL_HANDLE sdkdom, > + PRL_HANDLE sdktargetdev); > > /* > * Log error description > @@ -754,7 +757,8 @@ prlsdkAddDomainHardDisksInfo(vzDriverPtr driver, PRL_HANDLE sdkdom, virDomainDef > pret = PrlVmDev_GetEmulatedType(hdd, &emulatedType); > prlsdkCheckRetGoto(pret, error); > > - if (PDT_USE_REAL_DEVICE != emulatedType && IS_CT(def)) { > + if (IS_CT(def) && > + prlsdkInBootList(sdkdom, hdd)) { > > if (!(fs = virDomainFSDefNew())) > goto error; > @@ -1551,6 +1555,75 @@ virFindDiskBootIndex(virDomainDefPtr def, virDomainDiskDevice type, int index) > return NULL; > } > > +static bool > +prlsdkInBootList(PRL_HANDLE sdkdom, > + PRL_HANDLE sdktargetdev) > +{ > + bool ret = false; > + PRL_RESULT pret; > + PRL_UINT32 bootNum; > + PRL_HANDLE bootDev = PRL_INVALID_HANDLE; > + PRL_BOOL inUse; > + PRL_DEVICE_TYPE sdkType; > + PRL_UINT32 sdkIndex, bootIndex; > + PRL_UINT32 pos, targetpos; > + PRL_HANDLE dev = PRL_INVALID_HANDLE; > + size_t i; > + > + pret = PrlVmDev_GetStackIndex(sdktargetdev, &targetpos); > + prlsdkCheckRetExit(pret, -1); > + > + pret = PrlVmCfg_GetBootDevCount(sdkdom, &bootNum); > + prlsdkCheckRetExit(pret, -1); > + > + if (bootNum > VIR_DOMAIN_MAX_BOOT_DEVS) { > + bootNum = VIR_DOMAIN_MAX_BOOT_DEVS; > + VIR_WARN("Too many boot devices"); > + } This is not nesessary. We have VIR_DOMAIN_MAX_BOOT_DEVS limit only if we want to present boot order in domain definition.

> + > + for (i = 0; i < bootNum; ++i) { > + pret = PrlVmCfg_GetBootDev(sdkdom, i, &bootDev); > + prlsdkCheckRetGoto(pret, cleanup); > + > + pret = PrlBootDev_IsInUse(bootDev, &inUse); > + prlsdkCheckRetGoto(pret, cleanup); > + > + if (!inUse) { > + PrlHandle_Free(bootDev); > + bootDev = PRL_INVALID_HANDLE; > + continue; > + } > + > + pret = PrlBootDev_GetSequenceIndex(bootDev, &bootIndex); > + prlsdkCheckRetGoto(pret, cleanup); bootIndex is not used

> + > + pret = PrlBootDev_GetType(bootDev, &sdkType); > + prlsdkCheckRetGoto(pret, cleanup); > + > + pret = PrlBootDev_GetIndex(bootDev, &sdkIndex); > + prlsdkCheckRetGoto(pret, cleanup); > + > + dev = prlsdkGetDevByDevIndex(sdkdom, sdkType, sdkIndex); > + if (dev == PRL_INVALID_HANDLE) > + goto cleanup; > + > + pret = PrlVmDev_GetStackIndex(dev, &pos); > + prlsdkCheckRetExit(pret, -1); I would use PrlVmDev_GetType and PrlVmDev_GetIndex for given device and compare obtained values against sdkType and sdkIndex. It is more straight forward.

> + > + PrlHandle_Free(bootDev); > + bootDev = PRL_INVALID_HANDLE; > + > + if (pos == targetpos) { > + ret = true; > + break; > + } > + } > + > + cleanup: > + PrlHandle_Free(dev); > + PrlHandle_Free(bootDev); > + return ret; > +} > static int > prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, > virDomainDefPtr def, int bootIndex) > @@ -3741,23 +3814,26 @@ prlsdkSetBootOrderCt(PRL_HANDLE sdkdom, virDomainDefPtr def) > size_t i; > PRL_HANDLE hdd = PRL_INVALID_HANDLE; > PRL_RESULT pret; > + bool rootfs = false; > int ret = -1; > > /* if we have root mounted we don't need to explicitly set boot order */ let's move this comment to 'if (!rootfs) {'

> for (i = 0; i < def->nfss; i++) { > + > + pret = prlsdkAddDeviceToBootList(sdkdom, i, PDE_HARD_DISK, i + 1); > + prlsdkCheckRetExit(pret, -1); As this relies on the fact that we add disks for filesystems first and then disks for disks let's add comment to prlsdkDoApplyConfig that this order is now significant.

> + > if (STREQ(def->fss[i]->dst, "/")) > - return 0; > + rootfs = true; > } > > - /* else set first hard disk as boot device */ > - pret = prlsdkAddDeviceToBootList(sdkdom, 0, PDE_HARD_DISK, 0); > - prlsdkCheckRetExit(pret, -1); > - > - pret = PrlVmCfg_GetHardDisk(sdkdom, 0, &hdd); > - prlsdkCheckRetExit(pret, -1); > + if (!rootfs) { > + pret = PrlVmCfg_GetHardDisk(sdkdom, 0, &hdd); > + prlsdkCheckRetExit(pret, -1); If we mix filesystems and disks then first harddisk will represent filesystem. So we should use def->nfss instead of 0. It is a matter of a distinct patch I guess, nevertheless let's fix this place too. Nikolay

> > - PrlVmDevHd_SetMountPoint(hdd, "/"); > - prlsdkCheckRetGoto(pret, cleanup); > + PrlVmDevHd_SetMountPoint(hdd, "/"); > + prlsdkCheckRetGoto(pret, cleanup); > + } > > ret = 0; > >