22.03.2016 16:56, Nikolay Shirokovskiy пишет:

The patch makes some refactoring of the existing code. Current boot order spec code makes very simple thing in somewhat obscure way. In case of VMs it sets the first hdd as the only bootable device. In case of CTs it doesn't touch the boot order at all if one of the filesystems is mounted to root. Otherwise like in case of VMs it sets the first hdd as the only bootable device and additionally sets this device mount point to root. Refactored code makes all this explicit.

The actual boot order support is simple. Common libvirt domain xml parsing code makes the exact ordering of disks devices as described in docs for boot ordering (disks are sorted by bus order first, device target second. Bus order is the order of disk buses appearence in original xml. Device targets order is alphabetical). We add devices in the same order and SDK designates device indexes sequentially for each device type. Thus device index is equal to its boot index. For example N-th cdrom in boot specification refers to sdk cdrom with it's device index N.

If there is no boot spec in xml the parsing code will add <boot dev='hdd'> for HVMs automatically and we backward compatibly set fist hdd as bootable.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 148 ++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 101 insertions(+), 47 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index c0fb4fb..528a253 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -87,6 +87,14 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0)

+#define prlsdkCheckRetExit(ret, code) \ + do { \ + if (PRL_FAILED(ret)) { \ + logPrlError(ret); \ + return code; \ + } \ + } while (0) + static PRL_RESULT logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) @@ -1988,13 +1996,9 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) }

if (!IS_CT(def)) { - if (def->os.nBootDevs != 1 || - def->os.bootDevs[0] != VIR_DOMAIN_BOOT_DISK || - def->os.init != NULL || def->os.initargv != NULL) { - + if (def->os.init != NULL || def->os.initargv != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("changing OS parameters is not supported " - "by vz driver")); + _("unsupported OS parameters")); return -1; } } else { @@ -2003,8 +2007,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) (def->os.initargv != NULL && def->os.initargv[0] != NULL)) {

virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("changing OS parameters is not supported " - "by vz driver")); + _("unsupported OS parameters")); return -1; } } @@ -2991,9 +2994,7 @@ static int prlsdkDelDisk(PRL_HANDLE sdkdom, int idx)

static int prlsdkAddDisk(vzConnPtr privconn, PRL_HANDLE sdkdom, - virDomainDiskDefPtr disk, - bool bootDisk, - bool isCt) + virDomainDiskDefPtr disk) { PRL_RESULT pret; PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE; @@ -3002,7 +3003,6 @@ static int prlsdkAddDisk(vzConnPtr privconn, PRL_MASS_STORAGE_INTERFACE_TYPE sdkbus; int idx; virDomainDeviceDriveAddressPtr drive; - PRL_UINT32 devIndex; PRL_DEVICE_TYPE devType; PRL_CLUSTERED_DEVICE_SUBTYPE scsiModel; char *dst = NULL; @@ -3133,21 +3133,6 @@ static int prlsdkAddDisk(vzConnPtr privconn, goto cleanup; }

- if (bootDisk) { - pret = PrlVmDev_GetIndex(sdkdisk, &devIndex); - prlsdkCheckRetGoto(pret, cleanup); - - if (prlsdkAddDeviceToBootList(sdkdom, devIndex, devType, 0) < 0) - goto cleanup; - - /* If we add physical device as a boot disk to container - * we have to specify mount point for it */ - if (isCt) { - pret = PrlVmDevHd_SetMountPoint(sdkdisk, "/"); - prlsdkCheckRetGoto(pret, cleanup); - } - } - return 0; cleanup: PrlHandle_Free(sdkdisk); @@ -3168,11 +3153,7 @@ prlsdkAttachVolume(vzConnPtr privconn, if (PRL_FAILED(waitJob(job))) goto cleanup;

- ret = prlsdkAddDisk(privconn, - privdom->sdkdom, - disk, - false, - IS_CT(dom->def)); + ret = prlsdkAddDisk(privconn, privdom->sdkdom, disk); if (ret == 0) { job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); if (PRL_FAILED(waitJob(job))) { @@ -3300,6 +3281,86 @@ prlsdkAddFS(PRL_HANDLE sdkdom, virDomainFSDefPtr fs) PrlHandle_Free(sdkdisk); return ret; } + +static int +prlsdkSetBootOrderCt(PRL_HANDLE sdkdom, virDomainDefPtr def) +{ + size_t i; + PRL_HANDLE hdd = PRL_INVALID_HANDLE; + PRL_RESULT pret; + int ret = -1; + + /* if we have root mounted we don't need to explicitly set boot order */ + for (i = 0; i < def->nfss; i++) { + if (STREQ(def->fss[i]->dst, "/")) + return 0; + } + + /* 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); + + PrlVmDevHd_SetMountPoint(hdd, "/"); + prlsdkCheckRetGoto(pret, cleanup); + + ret = 0; + + cleanup: + PrlHandle_Free(hdd); + return ret; +} + +static int +prlsdkSetBootOrderVm(PRL_HANDLE sdkdom, virDomainDefPtr def) +{ + size_t i; + int idx[VIR_DOMAIN_BOOT_LAST] = { 0 }; + int bootIndex = 0; + PRL_RESULT pret; + PRL_UINT32 num; + int sdkType; + virDomainBootOrder virType; + + for (i = 0; i < def->os.nBootDevs; ++i) { + virType = def->os.bootDevs[i]; + + switch (virType) { + case VIR_DOMAIN_BOOT_CDROM: + sdkType = PDE_OPTICAL_DISK; + break; + case VIR_DOMAIN_BOOT_DISK: + sdkType = PDE_HARD_DISK; + break; + case VIR_DOMAIN_BOOT_NET: + sdkType = PDE_GENERIC_NETWORK_ADAPTER; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported boot device type: '%s'"), + virDomainBootTypeToString(virType)); + return -1; + } + + pret = PrlVmCfg_GetDevsCountByType(sdkdom, sdkType, &num); + prlsdkCheckRetExit(pret, -1); + + if (idx[virType] >= num) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Too many devices of type '%s' in boot spec"), + virDomainBootTypeToString(virType)); + return -1; + }

I would ease this strictness and remove the check as we didn't have it before and it actually doesn't solve any potential problem and only prohibits what was allowed before.

+ + pret = prlsdkAddDeviceToBootList(sdkdom, idx[virType]++, sdkType, bootIndex++); + prlsdkCheckRetExit(pret, -1); + } + + return 0; +} + static int prlsdkDoApplyConfig(virConnectPtr conn, PRL_HANDLE sdkdom, @@ -3309,7 +3370,6 @@ prlsdkDoApplyConfig(virConnectPtr conn, PRL_RESULT pret; size_t i; char uuidstr[VIR_UUID_STRING_BUFLEN + 2]; - bool needBoot = true; char *mask = NULL;

if (prlsdkCheckUnsupportedParams(sdkdom, def) < 0) @@ -3388,26 +3448,20 @@ prlsdkDoApplyConfig(virConnectPtr conn, }

for (i = 0; i < def->nfss; i++) { - if (STREQ(def->fss[i]->dst, "/")) - needBoot = false; if (prlsdkAddFS(sdkdom, def->fss[i]) < 0) goto error; }

for (i = 0; i < def->ndisks; i++) { - bool bootDisk = false; - - if (needBoot && - def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (prlsdkAddDisk(conn->privateData, sdkdom, def->disks[i]) < 0) + goto error; + }

- needBoot = false; - bootDisk = true; - } - if (prlsdkAddDisk(conn->privateData, - sdkdom, - def->disks[i], - bootDisk, - IS_CT(def)) < 0) + if (IS_CT(def)) { + if (prlsdkSetBootOrderCt(sdkdom, def) < 0) + goto error; + } else { + if (prlsdkSetBootOrderVm(sdkdom, def) < 0) goto error; }

22.03.2016 16:56, Nikolay Shirokovskiy пишет:

We want to report boot order in dumpxml for vz domains. Thus we want disks devices to be sorted in output compatible with boot ordering specification. So let's just use virDomainDiskInsert which makes appropriate sorting.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 528a253..f059a8e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -663,7 +663,7 @@ prlsdkAddDomainHardDisksInfo(vzConnPtr privconn, PRL_HANDLE sdkdom, virDomainDef if (prlsdkGetDiskInfo(privconn, hdd, disk, false, IS_CT(def)) < 0) goto error;

- if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) + if (virDomainDiskInsert(def, disk) < 0) goto error;

disk = NULL; @@ -706,7 +706,7 @@ prlsdkAddDomainOpticalDisksInfo(vzConnPtr privconn, PRL_HANDLE sdkdom, virDomain PrlHandle_Free(cdrom); cdrom = PRL_INVALID_HANDLE;

- if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) + if (virDomainDiskInsert(def, disk) < 0) goto error; }

ACK

22.03.2016 16:56, Nikolay Shirokovskiy пишет:

As usual we try to deal correctly with vz domains that were created by other means and thus can have all range of SDK domain parameters. If vz domain boot order can't be represented in libvirt os boot section let's give warning and make os boot section represent SDK to some extent.

1. Os boot section supports up to 4 boot devices. Here we just cut SDK boot order up to this limit. Not too bad.

2. If there is a floppy in boot order let's just skip it. Anyway we don't show it in the xml. Not too bad too.

3. SDK boot order with unsupported disks order. Say we have "hdb, hda" in SDK. We can not present this thru os boot order. Well let's just give warning but leave double <boot dev='hd'/> in xml. It's kind of misleading but we warn you!

SDK boot order have an extra parameters 'inUse' and 'sequenceIndex' which makes our task more complicated. In realitly however 'inUse' is always on and 'sequenceIndex == boot position index + 1' which simplifies out task back again! To be on a safe side let's explicitly check for this conditions!

We have another exercise here. We want to check for unrepresentable condition 3 (see above). The tricky part is that in contrast to domains defined thru this driver 3-rd party defined domains can have device ordering different from default. Thus we need some id to check that N-th boot disk of os boot section is same as N-th boot disk of SDK boot. This is what prlsdkBootOrderCheck for. It uses disks sources paths as id for disks and iface names for network devices.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 238 insertions(+)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index f059a8e..6cecb93 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1273,6 +1273,240 @@ prlsdkNewDomainByHandle(vzConnPtr privconn, PRL_HANDLE sdkdom) return dom; }

+static PRL_HANDLE +prlsdkGetDevByDevIndex(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE type, PRL_UINT32 devIndex) +{ + PRL_RESULT pret; + PRL_UINT32 index, num; + PRL_HANDLE dev = PRL_INVALID_HANDLE; + size_t i; + + pret = PrlVmCfg_GetDevsCountByType(sdkdom, type, &num); + prlsdkCheckRetGoto(pret, error); + + for (i = 0; i < num; ++i) { + pret = PrlVmCfg_GetDevByType(sdkdom, type, i, &dev); + prlsdkCheckRetGoto(pret, error); + + pret = PrlVmDev_GetIndex(dev, &index); + prlsdkCheckRetGoto(pret, error); + + if (index == devIndex) + break; + + PrlHandle_Free(dev); + dev = PRL_INVALID_HANDLE; + } + + return dev; + + error: + PrlHandle_Free(dev); + return PRL_INVALID_HANDLE; +} + +static virDomainDiskDefPtr +virFindDiskBootIndex(virDomainDefPtr def, virDomainDiskDevice type, int index) +{ + size_t i; + int c = 0; + + for (i = 0; i < def->ndisks; ++i) { + if (def->disks[i]->device != type) + continue; + if (c == index) + return def->disks[i]; + ++c; + } + + return NULL; +} + +static int +prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, + virDomainDefPtr def, int bootIndex) +{ + PRL_RESULT pret; + char *sdkName = NULL; + const char *bootName; + PRL_UINT32 buflen = 0; + PRL_HANDLE dev = PRL_INVALID_HANDLE; + virDomainDiskDefPtr disk; + virDomainDiskDevice device; + int ret = -1; + + dev = prlsdkGetDevByDevIndex(sdkdom, sdkType, sdkIndex); + if (dev == PRL_INVALID_HANDLE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Can't find boot device of type: %d, device index: %d"), + sdkType, sdkIndex); + return -1; + } + + switch (sdkType) { + case PDE_OPTICAL_DISK: + case PDE_HARD_DISK: + pret = PrlVmDev_GetFriendlyName(dev, sdkName, &buflen); + prlsdkCheckRetGoto(pret, cleanup); + + if (VIR_ALLOC_N(sdkName, buflen) < 0) + goto cleanup; + + pret = PrlVmDev_GetFriendlyName(dev, sdkName, &buflen); + prlsdkCheckRetGoto(pret, cleanup); + + switch (sdkType) { + case PDE_OPTICAL_DISK: + device = VIR_DOMAIN_DISK_DEVICE_CDROM; + break; + case PDE_HARD_DISK: + device = VIR_DOMAIN_DISK_DEVICE_DISK; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported disk type %d"), sdkType); + goto cleanup; + } + + if (!(disk = virFindDiskBootIndex(def, device, bootIndex))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Can find boot device of type: %s, index: %d"), + virDomainDiskDeviceTypeToString(device), bootIndex); + goto cleanup; + } + + bootName = disk->src->path; + + break; + case PDE_GENERIC_NETWORK_ADAPTER: + pret = PrlVmDevNet_GetHostInterfaceName(dev, NULL, &buflen); + prlsdkCheckRetGoto(pret, cleanup); + + if (VIR_ALLOC_N(sdkName, buflen) < 0) + goto cleanup; + + pret = PrlVmDevNet_GetHostInterfaceName(dev, sdkName, &buflen); + prlsdkCheckRetGoto(pret, cleanup); + + if (bootIndex >= def->nnets) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Can find network boot device for index: %d"), + bootIndex); + goto cleanup; + } + + bootName = def->nets[bootIndex]->ifname; + + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unexpected device type %d"), sdkType); + goto cleanup; + } + + if (STRNEQ(sdkName, bootName)) + VIR_WARN("Unrepresentable boot order configuration"); + + ret = 0; + + cleanup: + + VIR_FREE(sdkName); + PrlHandle_Free(dev); + return ret; +} + +static int +prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def) +{ + int ret = -1; + PRL_RESULT pret; + PRL_UINT32 bootNum; + PRL_HANDLE bootDev = PRL_INVALID_HANDLE; + PRL_BOOL inUse; + PRL_DEVICE_TYPE sdkType; + virDomainBootOrder type; + PRL_UINT32 bootIndex, sdkIndex; + int bootUsage[VIR_DOMAIN_BOOT_LAST] = { 0 }; + size_t i; + + pret = PrlVmCfg_GetBootDevCount(sdkdom, &bootNum); + prlsdkCheckRetExit(pret, -1); + + def->os.nBootDevs = 0; + + if (bootNum > VIR_DOMAIN_MAX_BOOT_DEVS) { + bootNum = VIR_DOMAIN_MAX_BOOT_DEVS; + VIR_WARN("Too many boot devices"); + } + + 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) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Boot ordering with disabled items is not supported")); + goto cleanup; + } + + pret = PrlBootDev_GetSequenceIndex(bootDev, &bootIndex); + prlsdkCheckRetGoto(pret, cleanup); + + /* bootIndex is started from 1 */ + if (bootIndex != i + 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported boot order configuration")); + goto cleanup; + } +

This check doesn't work because boot indexes are not necessarily continuous. The only condition we should check here is that they should be growing. I added the following chunk to your code and if you don't mind I can squash it to your patch and push. @@ -1432,7 +1432,7 @@ prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def) PRL_BOOL inUse; PRL_DEVICE_TYPE sdkType; virDomainBootOrder type; - PRL_UINT32 bootIndex, sdkIndex; + PRL_UINT32 prevBootIndex = 0, bootIndex, sdkIndex; int bootUsage[VIR_DOMAIN_BOOT_LAST] = { 0 }; size_t i; @@ -1463,11 +1463,12 @@ prlsdkConvertBootOrder(PRL_HANDLE sdkdom, virDomainDefPtr def) prlsdkCheckRetGoto(pret, cleanup); /* bootIndex is started from 1 */ - if (bootIndex != i + 1) { + if (bootIndex <= prevBootIndex) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unsupported boot order configuration")); goto cleanup; } + prevBootIndex = bootIndex; pret = PrlBootDev_GetType(bootDev, &sdkType); prlsdkCheckRetGoto(pret, cleanup);

+ pret = PrlBootDev_GetType(bootDev, &sdkType); + prlsdkCheckRetGoto(pret, cleanup); + + if (sdkType == PDE_FLOPPY_DISK) { + VIR_WARN("Skipping floppy from boot order."); + continue; + } + + switch (sdkType) { + case PDE_OPTICAL_DISK: + type = VIR_DOMAIN_BOOT_CDROM; + break; + case PDE_HARD_DISK: + type = VIR_DOMAIN_BOOT_DISK; + break; + case PDE_GENERIC_NETWORK_ADAPTER: + type = VIR_DOMAIN_BOOT_NET; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unexpected boot device type %i"), sdkType); + goto cleanup; + } + + pret = PrlBootDev_GetIndex(bootDev, &sdkIndex); + prlsdkCheckRetGoto(pret, cleanup); + + if (prlsdkBootOrderCheck(sdkdom, sdkType, sdkIndex, def, bootUsage[type]) < 0) + goto cleanup; + + bootUsage[type]++; + def->os.bootDevs[def->os.nBootDevs++] = type; + + PrlHandle_Free(bootDev); + bootDev = PRL_INVALID_HANDLE; + } + + ret = 0; + + cleanup: + PrlHandle_Free(bootDev); + return ret; +} + int prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom) { @@ -1328,6 +1562,10 @@ prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom) if (prlsdkAddDomainHardware(privconn, sdkdom, def) < 0) goto error;

+ /* depends on prlsdkAddDomainHardware */ + if (prlsdkConvertBootOrder(sdkdom, def) < 0) + goto error; + if (prlsdkAddVNCInfo(sdkdom, def) < 0) goto error;