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

[Maxim Nestratov]

21-Dec-16 10:25, Nikolay Shirokovskiy пишет:

>
> 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 at virtuozzo.com>
>> ---
>>   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.

Ok. Seems to be true.

>> +
>> +    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

Yeah, my first intention was to return it, then I reconsidered it but left the variable.

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

Ok. I'll try to do it this way.

>
>> +
>> +        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) {'

Ok

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

Makes sense

>
>> +
>>           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

Agree. See in the next version

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