[libvirt] [PATCH REBASE 0/4] vz: use disk bus and target name as disk id
This series is a preparation to implement device update functionality. In case of disk updates disk bus and disk target name pair is considered disk id. We already have places in vz driver code where we use disk source as id. These places are either not critical (see boot patch) or correct (see detach patch) so we need not to fix them immediately to use more rubust id. But as this id will be introduced anyway for disk updates let's move to new id in existing code. This way we have single id for disks. Nikolay Shirokovskiy (4): vz: introduce vzsdk disk id function vz: fix detach disk to use new disk id vz: fix boot check to use new disk id vz: cleanup: remove trivial function src/vz/vz_sdk.c | 251 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 126 insertions(+), 125 deletions(-) -- 1.8.3.1
Our intention is to use disk bus and disk target name pair as disk id instead of name returned by PrlVmDev_GetFriendlyName. We already have the code that extracts this pair from vzsdk data. Let's factor it out into a function. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 89 ++++++++++++++++++++++++++++++--------------------------- 1 file changed, 47 insertions(+), 42 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 01de4f6..9c18abb 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -486,6 +486,45 @@ prlsdkAddDomainVideoInfo(PRL_HANDLE sdkdom, virDomainDefPtr def) } static int +prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char **dst) +{ + PRL_RESULT pret; + PRL_UINT32 pos, ifType; + + pret = PrlVmDev_GetStackIndex(disk, &pos); + prlsdkCheckRetExit(pret, -1); + + /* Let physical devices added to CT look like SATA disks */ + if (isCt) { + ifType = PMS_SATA_DEVICE; + } else { + pret = PrlVmDev_GetIfaceType(disk, &ifType); + prlsdkCheckRetExit(pret, -1); + } + + switch (ifType) { + case PMS_IDE_DEVICE: + *bus = VIR_DOMAIN_DISK_BUS_IDE; + *dst = virIndexToDiskName(pos, "hd"); + break; + case PMS_SCSI_DEVICE: + *bus = VIR_DOMAIN_DISK_BUS_SCSI; + *dst = virIndexToDiskName(pos, "sd"); + break; + case PMS_SATA_DEVICE: + *bus = VIR_DOMAIN_DISK_BUS_SATA; + *dst = virIndexToDiskName(pos, "sd"); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown disk bus: %X"), ifType); + return -1; + } + + return 0; +} + +static int prlsdkGetDiskInfo(vzDriverPtr driver, PRL_HANDLE prldisk, virDomainDiskDefPtr disk, @@ -495,9 +534,8 @@ prlsdkGetDiskInfo(vzDriverPtr driver, char *buf = NULL; PRL_RESULT pret; PRL_UINT32 emulatedType; - PRL_UINT32 ifType; - PRL_UINT32 pos; virDomainDeviceDriveAddressPtr address; + int busIdx, devIdx; int ret = -1; pret = PrlVmDev_GetEmulatedType(prldisk, &emulatedType); @@ -530,50 +568,17 @@ prlsdkGetDiskInfo(vzDriverPtr driver, if (virDomainDiskSetSource(disk, buf) < 0) goto cleanup; - /* Let physical devices added to CT look like SATA disks */ - if (isCt) { - ifType = PMS_SATA_DEVICE; - } else { - pret = PrlVmDev_GetIfaceType(prldisk, &ifType); - prlsdkCheckRetGoto(pret, cleanup); - } - - pret = PrlVmDev_GetStackIndex(prldisk, &pos); - prlsdkCheckRetGoto(pret, cleanup); - - address = &disk->info.addr.drive; - switch (ifType) { - case PMS_IDE_DEVICE: - disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - disk->dst = virIndexToDiskName(pos, "hd"); - address->bus = pos / 2; - address->target = 0; - address->unit = pos % 2; - break; - case PMS_SCSI_DEVICE: - disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; - disk->dst = virIndexToDiskName(pos, "sd"); - address->bus = 0; - address->target = 0; - address->unit = pos; - break; - case PMS_SATA_DEVICE: - disk->bus = VIR_DOMAIN_DISK_BUS_SATA; - disk->dst = virIndexToDiskName(pos, "sd"); - address->bus = 0; - address->target = 0; - address->unit = pos; - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown disk bus: %X"), ifType); + if (prlsdkGetDiskId(prldisk, isCt, &disk->bus, &disk->dst) < 0) goto cleanup; - break; - } - if (!disk->dst) + if (virDiskNameToBusDeviceIndex(disk, &busIdx, &devIdx) < 0) goto cleanup; + address = &disk->info.addr.drive; + address->bus = busIdx; + address->target = 0; + address->unit = devIdx; + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; ret = 0; -- 1.8.3.1
14.04.2016 13:45, Nikolay Shirokovskiy пишет:
Our intention is to use disk bus and disk target name pair as disk id instead of name returned by PrlVmDev_GetFriendlyName. We already have the code that extracts this pair from vzsdk data. Let's factor it out into a function.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 89 ++++++++++++++++++++++++++++++--------------------------- 1 file changed, 47 insertions(+), 42 deletions(-)
ACK
Actually using disk PrlVmDev_GetFriendlyName as id on detaching volumes is not a problem. We can only detach hard disks and these can not have empty friendly names. But upcoming update device functionality for cdroms can not use disk source as id at all as update operation typically change this same source value. Thus we will need to use cdrom bus and cdrom target name as cdrom id. So in attempt to use same id scheme for all purpuses lets fix hard disk detach function to use new id. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 106 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 50 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 9c18abb..b2eb461 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3335,6 +3335,47 @@ static int prlsdkAddDisk(vzDriverPtr driver, return ret; } +static PRL_HANDLE +prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk, bool isCt) +{ + PRL_RESULT pret; + PRL_UINT32 hddCount; + size_t i; + PRL_HANDLE hdd = PRL_INVALID_HANDLE; + int bus; + char *dst = NULL; + + pret = PrlVmCfg_GetHardDisksCount(sdkdom, &hddCount); + prlsdkCheckRetGoto(pret, error); + + for (i = 0; i < hddCount; ++i) { + pret = PrlVmCfg_GetHardDisk(sdkdom, i, &hdd); + prlsdkCheckRetGoto(pret, error); + + if (prlsdkGetDiskId(hdd, isCt, &bus, &dst) < 0) + goto error; + + if (disk->bus == bus && STREQ(disk->dst, dst)) { + VIR_FREE(dst); + return hdd; + } + + PrlHandle_Free(hdd); + hdd = PRL_INVALID_HANDLE; + VIR_FREE(dst); + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No disk with bus '%s' and target '%s'"), + virDomainDiskBusTypeToString(disk->bus), disk->dst); + return PRL_INVALID_HANDLE; + + error: + VIR_FREE(dst); + PrlHandle_Free(hdd); + return PRL_INVALID_HANDLE; +} + int prlsdkAttachVolume(vzDriverPtr driver, virDomainObjPtr dom, @@ -3361,70 +3402,35 @@ prlsdkAttachVolume(vzDriverPtr driver, return ret; } -static int -prlsdkGetDiskIndex(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) -{ - int idx = -1; - char *buf = NULL; - PRL_RESULT pret; - PRL_UINT32 hddCount; - PRL_UINT32 i; - PRL_HANDLE hdd = PRL_INVALID_HANDLE; - - pret = PrlVmCfg_GetHardDisksCount(sdkdom, &hddCount); - prlsdkCheckRetGoto(pret, cleanup); - - for (i = 0; i < hddCount; ++i) { - - pret = PrlVmCfg_GetHardDisk(sdkdom, i, &hdd); - prlsdkCheckRetGoto(pret, cleanup); - - if (!(buf = prlsdkGetStringParamVar(PrlVmDev_GetFriendlyName, hdd))) - goto cleanup; - - if (STRNEQ(disk->src->path, buf)) { - - PrlHandle_Free(hdd); - hdd = PRL_INVALID_HANDLE; - VIR_FREE(buf); - continue; - } - - VIR_FREE(buf); - idx = i; - break; - } - - cleanup: - PrlHandle_Free(hdd); - return idx; -} - int prlsdkDetachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk) { - int ret = -1, idx; + int ret = -1; vzDomObjPtr privdom = dom->privateData; PRL_HANDLE job = PRL_INVALID_HANDLE; + PRL_HANDLE sdkdisk; + PRL_RESULT pret; - idx = prlsdkGetDiskIndex(privdom->sdkdom, disk); - if (idx < 0) + sdkdisk = prlsdkGetDisk(privdom->sdkdom, disk, IS_CT(dom->def)); + if (sdkdisk == PRL_INVALID_HANDLE) goto cleanup; job = PrlVm_BeginEdit(privdom->sdkdom); if (PRL_FAILED(waitJob(job))) goto cleanup; - ret = prlsdkDelDisk(privdom->sdkdom, idx); - if (ret == 0) { - job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); - if (PRL_FAILED(waitJob(job))) { - ret = -1; - goto cleanup; - } - } + pret = PrlVmDev_Remove(sdkdisk); + prlsdkCheckRetGoto(pret, cleanup); + + job = PrlVm_CommitEx(privdom->sdkdom, PVCF_DETACH_HDD_BUNDLE); + if (PRL_FAILED(waitJob(job))) + goto cleanup; + + ret = 0; cleanup: + + PrlHandle_Free(sdkdisk); return ret; } -- 1.8.3.1
14.04.2016 13:45, Nikolay Shirokovskiy пишет:
Actually using disk PrlVmDev_GetFriendlyName as id on detaching volumes is not a problem. We can only detach hard disks and these can not have empty friendly names. But upcoming update device functionality for cdroms can not use disk source as id at all as update operation typically change this same source value. Thus we will need to use cdrom bus and cdrom target name as cdrom id. So in attempt to use same id scheme for all purpuses lets fix hard disk detach function to use new id.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 106 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 50 deletions(-)
ACK
Current implementation does not detects all incompatible configurations. For example if we have in vzsdk bootorder "cdrom1, cdrom0" (that is "hdb, hda" in case of ide cdroms) and cdroms do not have disk images inserted. In this case boot order check code fail to distiguish them at all as for both PrlVmDev_GetFriendlyName gives "". Well the consequences are only missing warnings but as we just have introduced all the necessary tools to face the problem - let's fix it. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b2eb461..4df2ca0 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1309,10 +1309,11 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, virDomainDefPtr def, int bootIndex) { char *sdkName = NULL; - const char *bootName; PRL_HANDLE dev = PRL_INVALID_HANDLE; virDomainDiskDefPtr disk; virDomainDiskDevice device; + int bus; + char *dst = NULL; int ret = -1; dev = prlsdkGetDevByDevIndex(sdkdom, sdkType, sdkIndex); @@ -1326,9 +1327,6 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, switch (sdkType) { case PDE_OPTICAL_DISK: case PDE_HARD_DISK: - if (!(sdkName = prlsdkGetStringParamVar(PrlVmDev_GetFriendlyName, dev))) - goto cleanup; - switch (sdkType) { case PDE_OPTICAL_DISK: device = VIR_DOMAIN_DISK_DEVICE_CDROM; @@ -1349,7 +1347,11 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, goto cleanup; } - bootName = disk->src->path; + if (prlsdkGetDiskId(dev, false, &bus, &dst) < 0) + goto cleanup; + + if (!(bus == disk->bus && STREQ(disk->dst, dst))) + VIR_WARN("Unrepresentable boot order configuration"); break; case PDE_GENERIC_NETWORK_ADAPTER: @@ -1364,7 +1366,8 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, goto cleanup; } - bootName = def->nets[bootIndex]->ifname; + if (STRNEQ(sdkName, def->nets[bootIndex]->ifname)) + VIR_WARN("Unrepresentable boot order configuration"); break; default: @@ -1373,15 +1376,13 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE sdkType, int sdkIndex, goto cleanup; } - if (STRNEQ(sdkName, bootName)) - VIR_WARN("Unrepresentable boot order configuration"); - ret = 0; cleanup: VIR_FREE(sdkName); PrlHandle_Free(dev); + VIR_FREE(dst); return ret; } -- 1.8.3.1
14.04.2016 13:45, Nikolay Shirokovskiy пишет:
Current implementation does not detects all incompatible configurations. For example if we have in vzsdk bootorder "cdrom1, cdrom0" (that is "hdb, hda" in case of ide cdroms) and cdroms do not have disk images inserted. In this case boot order check code fail to distiguish them at all as for both PrlVmDev_GetFriendlyName gives "". Well the consequences are only missing warnings but as we just have introduced all the necessary tools to face the problem - let's fix it.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/vz/vz_sdk.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
ACK
--- src/vz/vz_sdk.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 4df2ca0..2e9544b 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3169,25 +3169,6 @@ int prlsdkDetachNet(vzDriverPtr driver, return ret; } -static int prlsdkDelDisk(PRL_HANDLE sdkdom, int idx) -{ - int ret = -1; - PRL_RESULT pret; - PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE; - - pret = PrlVmCfg_GetHardDisk(sdkdom, idx, &sdkdisk); - prlsdkCheckRetGoto(pret, cleanup); - - pret = PrlVmDev_Remove(sdkdisk); - prlsdkCheckRetGoto(pret, cleanup); - - ret = 0; - - cleanup: - PrlHandle_Free(sdkdisk); - return ret; -} - static int prlsdkAddDisk(vzDriverPtr driver, PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) @@ -3809,6 +3790,7 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) PRL_UINT32 hddCount; PRL_UINT32 i; PRL_HANDLE job; + PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE; job = PrlVm_BeginEdit(sdkdom); if (PRL_FAILED(waitJob(job))) @@ -3818,17 +3800,24 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) prlsdkCheckRetGoto(pret, cleanup); for (i = 0; i < hddCount; ++i) { - ret = prlsdkDelDisk(sdkdom, i); - if (ret) - goto cleanup; + pret = PrlVmCfg_GetHardDisk(sdkdom, i, &sdkdisk); + prlsdkCheckRetGoto(pret, cleanup); + + pret = PrlVmDev_Remove(sdkdisk); + prlsdkCheckRetGoto(pret, cleanup); + + PrlHandle_Free(sdkdisk); + sdkdisk = PRL_INVALID_HANDLE; } job = PrlVm_CommitEx(sdkdom, PVCF_DETACH_HDD_BUNDLE); if (PRL_FAILED(waitJob(job))) - ret = -1; + goto cleanup; - cleanup: + ret = 0; + cleanup: + PrlHandle_Free(sdkdisk); return ret; } -- 1.8.3.1
14.04.2016 13:45, Nikolay Shirokovskiy пишет:
--- src/vz/vz_sdk.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 4df2ca0..2e9544b 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -3169,25 +3169,6 @@ int prlsdkDetachNet(vzDriverPtr driver, return ret; }
-static int prlsdkDelDisk(PRL_HANDLE sdkdom, int idx) -{ - int ret = -1; - PRL_RESULT pret; - PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE; - - pret = PrlVmCfg_GetHardDisk(sdkdom, idx, &sdkdisk); - prlsdkCheckRetGoto(pret, cleanup); - - pret = PrlVmDev_Remove(sdkdisk); - prlsdkCheckRetGoto(pret, cleanup); - - ret = 0; - - cleanup: - PrlHandle_Free(sdkdisk); - return ret; -} - static int prlsdkAddDisk(vzDriverPtr driver, PRL_HANDLE sdkdom, virDomainDiskDefPtr disk) @@ -3809,6 +3790,7 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) PRL_UINT32 hddCount; PRL_UINT32 i; PRL_HANDLE job; + PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE;
job = PrlVm_BeginEdit(sdkdom); if (PRL_FAILED(waitJob(job))) @@ -3818,17 +3800,24 @@ prlsdkDetachDomainHardDisks(PRL_HANDLE sdkdom) prlsdkCheckRetGoto(pret, cleanup);
for (i = 0; i < hddCount; ++i) { - ret = prlsdkDelDisk(sdkdom, i); - if (ret) - goto cleanup; + pret = PrlVmCfg_GetHardDisk(sdkdom, i, &sdkdisk); + prlsdkCheckRetGoto(pret, cleanup); + + pret = PrlVmDev_Remove(sdkdisk); + prlsdkCheckRetGoto(pret, cleanup); + + PrlHandle_Free(sdkdisk); + sdkdisk = PRL_INVALID_HANDLE; }
job = PrlVm_CommitEx(sdkdom, PVCF_DETACH_HDD_BUNDLE); if (PRL_FAILED(waitJob(job))) - ret = -1; + goto cleanup;
- cleanup: + ret = 0;
+ cleanup: + PrlHandle_Free(sdkdisk); return ret; }
ACK
14.04.2016 13:45, Nikolay Shirokovskiy пишет:
This series is a preparation to implement device update functionality. In case of disk updates disk bus and disk target name pair is considered disk id. We already have places in vz driver code where we use disk source as id. These places are either not critical (see boot patch) or correct (see detach patch) so we need not to fix them immediately to use more rubust id. But as this id will be introduced anyway for disk updates let's move to new id in existing code. This way we have single id for disks.
Nikolay Shirokovskiy (4): vz: introduce vzsdk disk id function vz: fix detach disk to use new disk id vz: fix boot check to use new disk id vz: cleanup: remove trivial function
src/vz/vz_sdk.c | 251 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 126 insertions(+), 125 deletions(-)
Pushed the series. Thanks. Maxim
participants (2)
-
Maxim Nestratov -
Nikolay Shirokovskiy