Re: [libvirt] [PATCH v2 06/12] qemu: Generate cmd line at startup

On 03/02/2018 02:22 PM, John Ferlan wrote: > > > On 02/21/2018 01:11 PM, Michal Privoznik wrote: >> This is the easier part. All we need to do here is put -object >> pr-manger-helper,id=$alias,path=$socketPath and then just >> reference the object in -drive file.pr-manger=$alias. > > s/manger/manager/ > > My fingers usually the same way though as manger > >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_command.c | 40 ++++++++++++++++++++++ >> .../disk-virtio-scsi-reservations-not-managed.args | 28 +++++++++++++++ >> .../disk-virtio-scsi-reservations.args | 29 ++++++++++++++++ >> tests/qemuxml2argvtest.c | 8 +++++ >> 4 files changed, 105 insertions(+) >> create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.args >> create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index fa0aa5d5c..069d60d35 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src) >> } >> >> >> +static void >> +qemuBuildDriveSourcePR(virBufferPtr buf, >> + virStorageSourcePtr src) >> +{ >> + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); >> + qemuDomainDiskPRDPtr prd = srcPriv->prd; >> + >> + if (!prd || !prd->alias) >> + return; >> + >> + virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias); >> +} >> + >> + >> static int >> qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, >> virQEMUCapsPtr qemuCaps, >> @@ -1590,6 +1604,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, >> >> if (disk->src->debug) >> virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel); >> + >> + qemuBuildDriveSourcePR(buf, disk->src); >> } else { >> if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops))) >> goto cleanup; >> @@ -9789,6 +9805,28 @@ qemuBuildPanicCommandLine(virCommandPtr cmd, >> } >> >> >> +static void >> +qemuBuildMasterPRCommandLine(virCommandPtr cmd, >> + const virDomainDef *def) >> +{ >> + size_t i; >> + >> + for (i = 0; i < def->ndisks; i++) { >> + const virDomainDiskDef *disk = def->disks[i]; >> + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); >> + qemuDomainDiskPRDPtr prd = srcPriv->prd; >> + virBuffer buf = VIR_BUFFER_INITIALIZER; >> + >> + if (!prd || !prd->alias) >> + continue; >> + >> + virBufferAsprintf(&buf, "pr-manager-helper,id=%s,path=%s", prd->alias, prd->path); >> + virCommandAddArg(cmd, "-object"); >> + virCommandAddArgBuffer(cmd, &buf); > > What happens when there's more than one disk using the managed mode > where you have a "static" alias and path, wouldn't there be multiple > lines with: > > -object > pr-manager-helper,id=pr-helper0,path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock Ah sure.

> > ? And how is QEMU going to react to that? > > IOW: Shouldn't this code know it's already created an object for that > case and not generate another one? > > The other one : > > -object pr-manager-helper,id=pr-helper-sdb,path=/path/to/qemu-pr-helper.sock > > I get, but I'm still not thrilled with "sdb" as opposed to the > disk->info.alias of "scsi0-0-0-0" more commonly used. IIRC, there's no > guarantee that what libvirt calls "sdb" ends up being "sdb" on the > guest. My vague recollection of the algorithm that "automagically" > generates the address based on sda, sdb, sdc, etc. So "sdb" IIRC would > related to an address that would create an alias using "0-0-1"; whereas, > "sda" would create that "0-0-0" value. Yes. I've changed it in the other patch so now 'pr-helper-scsi0-0-0-0' is generated. Michal