Re: [libvirt] [PATCH 2/2] vz: support filesystem type volume

On 08.04.2016 19:39, Olga Krishtal wrote: > Vz containers are able to use ploop volumes from storage pools > to work upon. > > To use filesystem type volume, pool name and volume name should be > specifaed in <source> > > Signed-off-by: Olga Krishtal <okrishtal(a)virtuozzo.com&gt; > --- > src/storage/storage_driver.c | 3 + > src/vz/vz_sdk.c | 127 ++++++++++++++++++++++++++++++++++--------- > 2 files changed, 105 insertions(+), 25 deletions(-) > > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index 1d96618..c2c1483 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -3348,6 +3348,9 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, > def->src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; > break; > > + case VIR_STORAGE_VOL_PLOOP: > + def->src->srcpool->actualtype = VIR_STORAGE_TYPE_FILE; > + > case VIR_STORAGE_VOL_NETWORK: > case VIR_STORAGE_VOL_NETDIR: > virReportError(VIR_ERR_INTERNAL_ERROR, > diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c > index 00f42b8..a8b2ffa 100644 > --- a/src/vz/vz_sdk.c > +++ b/src/vz/vz_sdk.c > @@ -31,6 +31,8 @@ > #include "datatypes.h" > #include "domain_conf.h" > #include "virtime.h" > +#include "dirname.h" seems we don't use it > +#include "storage/storage_driver.h" > > #include "vz_sdk.h" > > @@ -570,8 +572,36 @@ prlsdkGetFSInfo(PRL_HANDLE prldisk, > PRL_UINT32 buflen = 0; > PRL_RESULT pret; > int ret = -1; > + char *storage = NULL; > + char **matches = NULL; > + virURIPtr uri = NULL; > + > + pret = PrlVmDevHd_GetStorageURL(prldisk, NULL, &buflen); > + prlsdkCheckRetGoto(pret, cleanup); > + if (VIR_ALLOC_N(storage, buflen) < 0) > + goto cleanup; > + pret = PrlVmDevHd_GetStorageURL(prldisk, storage, &buflen); > + prlsdkCheckRetGoto(pret, cleanup); there is brand new prlsdkGetStringParamVar for getting strings from sdk

> + > + if (!virStringIsEmpty(storage)) { > + uri = virURIParse(storage); > + if (!uri || STRNEQ("volume", uri->scheme)) > + goto cleanup; second clause need error message

> + if (!(matches = virStringSplitCount(uri->path, "/", 0, NULL))) > + goto cleanup; check count or we have invalid pointers in matches[N] or add matches[0] to the check.

> + if (!matches[1] || !matches[2]) > + goto cleanup; > + fs->type = VIR_DOMAIN_FS_TYPE_VOLUME; > + if (VIR_ALLOC(fs->src->srcpool) < 0) > + goto cleanup; > + if (VIR_STRDUP(fs->src->srcpool->pool, matches[1]) < 0) > + goto cleanup; > + if (VIR_STRDUP(fs->src->srcpool->volume, matches[2]) < 0) > + goto cleanup; > + } else { > + fs->type = VIR_DOMAIN_FS_TYPE_FILE; i think we should move setting fs->src->path under this branch, we don't need this set in case of volume AFAIU

> + } > > - fs->type = VIR_DOMAIN_FS_TYPE_FILE; > fs->fsdriver = VIR_DOMAIN_FS_DRIVER_TYPE_PLOOP; > fs->accessmode = VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH; > fs->wrpolicy = VIR_DOMAIN_FS_WRPOLICY_DEFAULT; > @@ -608,6 +638,9 @@ prlsdkGetFSInfo(PRL_HANDLE prldisk, > > cleanup: > VIR_FREE(buf); > + VIR_FREE(storage); > + virURIFree(uri); > + virStringFreeList(matches); > return ret; > } > > @@ -636,7 +669,7 @@ prlsdkAddDomainHardDisksInfo(vzConnPtr privconn, PRL_HANDLE sdkdom, virDomainDef > > if (PDT_USE_REAL_DEVICE != emulatedType && IS_CT(def)) { > > - if (VIR_ALLOC(fs) < 0) > + if (!(fs = virDomainFSDefNew())) > goto error; > > if (prlsdkGetFSInfo(hdd, fs) < 0) > @@ -2417,13 +2450,14 @@ static int prlsdkCheckNetUnsupportedParams(virDomainNetDefPtr net) > > static int prlsdkCheckFSUnsupportedParams(virDomainFSDefPtr fs) > { > - if (fs->type != VIR_DOMAIN_FS_TYPE_FILE) { > + if (fs->type != VIR_DOMAIN_FS_TYPE_FILE && > + fs->type != VIR_DOMAIN_FS_TYPE_VOLUME) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("Only file based filesystems are " > - "supported by vz driver.")); > + _("Unsupported filesystem type.")); > return -1; > } > > + unrelated, stricly speaking )

> if (fs->fsdriver != VIR_DOMAIN_FS_DRIVER_TYPE_PLOOP) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("Only ploop fs driver is " > @@ -3266,6 +3300,7 @@ prlsdkAddFS(PRL_HANDLE sdkdom, virDomainFSDefPtr fs) > PRL_RESULT pret; > PRL_HANDLE sdkdisk = PRL_INVALID_HANDLE; > int ret = -1; > + char *storage = NULL; > > if (prlsdkCheckFSUnsupportedParams(fs) < 0) > return -1; > @@ -3273,6 +3308,13 @@ prlsdkAddFS(PRL_HANDLE sdkdom, virDomainFSDefPtr fs) > pret = PrlVmCfg_CreateVmDev(sdkdom, PDE_HARD_DISK, &sdkdisk); > prlsdkCheckRetGoto(pret, cleanup); > > + if (fs->type == VIR_DOMAIN_FS_TYPE_VOLUME) { > + if (virAsprintf(&storage, "volume///%s/%s", fs->src->srcpool->pool, volume:

> + fs->src->srcpool->volume) < 0) > + goto cleanup; > + pret = PrlVmDevHd_SetStorageURL(sdkdisk, storage); > + prlsdkCheckRetGoto(pret, cleanup); > + } > pret = PrlVmDev_SetEnabled(sdkdisk, 1); > prlsdkCheckRetGoto(pret, cleanup); > > @@ -3297,6 +3339,7 @@ prlsdkAddFS(PRL_HANDLE sdkdom, virDomainFSDefPtr fs) > ret = 0; > > cleanup: > + VIR_FREE(storage); > PrlHandle_Free(sdkdisk); > return ret; > } > @@ -3388,10 +3431,10 @@ 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; > + if (STREQ(def->fss[i]->dst, "/")) > + needBoot = false; > + if (prlsdkAddFS(sdkdom, def->fss[i]) < 0) > + goto error; unrelated

> } > > for (i = 0; i < def->ndisks; i++) { > @@ -3493,6 +3536,49 @@ prlsdkCreateVm(virConnectPtr conn, virDomainDefPtr def) > return ret; > } > > +static int > +virStorageTranslatePoolLocal(virConnectPtr conn, virStorageSourcePtr src) > +{ > + > + virStoragePoolPtr pool = NULL; > + virStorageVolPtr vol = NULL; > + virStorageVolInfo info; > + int ret = -1; > + > + if (!(pool = virStoragePoolLookupByName(conn, src->srcpool->pool))) > + return -1; > + if (virStoragePoolIsActive(pool) != 1) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("storage pool '%s' containing volume '%s' " > + "is not active"), src->srcpool->pool, > + src->srcpool->volume); > + goto cleanup; > + } > + > + if (!(vol = virStorageVolLookupByName(pool, src->srcpool->volume))) > + goto cleanup; > + > + if (virStorageVolGetInfo(vol, &info) < 0) > + goto cleanup; > + > + if (info.type != VIR_STORAGE_VOL_PLOOP) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Unsupported volume format '%s'"), > + virStorageVolTypeToString(info.type)); > + goto cleanup; > + } > + > + if (!(src->path = virStorageVolGetPath(vol))) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + virObjectUnref(pool); > + virObjectUnref(vol); > + return ret; > +} > + > int > prlsdkCreateCt(virConnectPtr conn, virDomainDefPtr def) > { > @@ -3506,25 +3592,16 @@ prlsdkCreateCt(virConnectPtr conn, virDomainDefPtr def) > int useTemplate = 0; > size_t i; > > - if (def->nfss > 1) { > - /* Check all filesystems */ > - for (i = 0; i < def->nfss; i++) { > - if (def->fss[i]->type != VIR_DOMAIN_FS_TYPE_FILE) { > - virReportError(VIR_ERR_INVALID_ARG, "%s", > - _("Unsupported filesystem type.")); > - return -1; > - } > - } > - } else if (def->nfss == 1) { > - if (def->fss[0]->type == VIR_DOMAIN_FS_TYPE_TEMPLATE) { > + if (def->nfss == 1 && def->fss[0]->type == VIR_DOMAIN_FS_TYPE_TEMPLATE) > useTemplate = 1; indentation

> - } else if (def->fss[0]->type != VIR_DOMAIN_FS_TYPE_FILE) { > - virReportError(VIR_ERR_INVALID_ARG, "%s", > - _("Unsupported filesystem type.")); > - return -1; you take benefits of prlsdkCheckFSUnsupportedParams checks and drop / * Check all filesystems */ above. This is not obvious and is not this patch intention. Please move to a distinct patch.

> + > + for (i = 0; i < def->nfss; i++) { > + if (def->fss[i]->type == VIR_DOMAIN_FS_TYPE_VOLUME) { > + if (virStorageTranslatePoolLocal(conn, def->fss[i]->src) < 0) > + goto cleanup; we don't need to tranlate at this point, please move to prlsdkAddFS

> } > - } > > + } > confParam.nVmType = PVT_CT; > confParam.sConfigSample = "vswap.1024MB"; > confParam.nOsVersion = 0; >