Re: [libvirt] [PATCH] qemu: Support to detach redirdev device
On 2016年03月05日 19:52, John Ferlan wrote:
On 02/27/2016 04:50 AM, Osier Yang wrote:
Attaching redirdev device has been supported for a while, but detaching
is not never implemented.
Simple procedure to test:
% lsusb
Bus 001 Device 014: ID 0781:5567 SanDisk Corp. Cruzer Blade
% usbredirserver -p 4000 0781:5567
% virsh attach-device test usb.xml
% cat usb.xml
% virsh detach-device test usb.xml
% virsh qemu-monitor-command test --pretty '{"execute": "query-chardev"}' |
grep 4000
On success, the chardev should not seen in output of above command.
---
src/conf/domain_conf.c | 67 +
src/conf/domain_conf.h | 4 ++
src/libvirt_private.syms | 3 ++
src/qemu/qemu_driver.c | 4 +-
src/qemu/qemu_hotplug.c | 97 +++-
src/qemu/qemu_hotplug.h | 3 ++
6 files changed, 176 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3b15cb4..d304232 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13825,6 +13825,73 @@ virDomainMemoryRemove(virDomainDefPtr def,
return ret;
}
A little intro - inputs and what it returns (-1 or the index into the
redirdevs for the found redirdev.
ok
+ssize_t
+virDomainRedirdevFind(virDomainDefPtr def,
+ virDomainRedirdevDefPtr redirdev)
I see you're essentially copying the virDomainRNGFind... and friends...
Honestly, yes. With thinking the existing code is good, I reused them
and produced the patch in 1 hour, with making sure the syntax-check
and building work well, and the redirdev device can be unhotpluged
successfully.
You might criticize for why not hack the existing bad code before making
a new patch, but I really don't have much time look around currently.
+{
+size_t i;
+
+for (i = 0; i < def->nredirdevs; i++) {
+virDomainRedirdevDefPtr tmp = def->redirdevs[i];
+
+if (redirdev->bus != tmp->bus)
+continue;
+
+virDomainChrSourceDef source_chr = redirdev->source.chr;
+virDomainChrSourceDef tmp_chr = tmp->source.chr;
+
+if (source_chr.type != tmp_chr.type)
+continue;
Does it matter if the was set in the XML? If it was
the boot device, then should it be allowed to be removed? Found yes,
but removed? I guess that decision is below us though.
Whether the device should be removed or not is not the business of this
function, which is just to find the index of the device in the list. And
I don't
even think libvirt should take care of it.
+
+switch (source_chr.type) {
+case VIR_DOMAIN_CHR_TYPE_TCP:
+if (STRNEQ_NULLABLE(source_chr.data.tcp.host,
+tmp_chr.data.tcp.host))
+continue;
+if (STRNEQ_NULLABLE(source_chr.data.tcp.service,
+tmp_chr.data.tcp.service))
+continue;
+if (source_chr.data.tcp.listen != tmp_chr.data.tcp.listen)
+continue;
+if (source_chr.data.tcp.protocol != tmp_chr.data.tcp.protocol)
+continue;
+break;
+
+case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
+if (source_chr.data.spicevmc != tmp_chr.data.spicevmc)
+continue;
+break;
+
+default:
+/* Unlikely, currently redirdev only supports character device of
+ * type "tcp" and "spicevmc".
+ */
Shouldn't this then be a continue; here? IOW: For anything not being
supported we don't want to take the next step, right? I know you're
following the RNG code...
Well, why to continue? Actually if it flows to this branch, something
must be
wrong with the @redirdev for which you want to find the index number, either
there is bug in the redirdev config parsing, or memory corruption in
libvirtd.
(Note that the condition for the switch statement is "source_chr.type").
So basically, you can either report error here, or just ignore it with
sensible
comments. Or better, something like "goto out;", to avoid the below
checking
(virDomainDeviceInfoAddressIsEqual).
If you really think it worths to take care of the situation unlikely
happens,
I'm fine to refactor it with "goto". Personally I don't think it worths.
+break;
+}
+
+if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
Don't think it matters for checking against TYPE_NONE since the
following function does check that... Again more RNG-alike...
+!virDomainDeviceInfoAddressIsEqual(>info, >info))
+continue;
Should I assume if we get this far then this is *the* device to be
removed? And there's only one, right?
Hence just "return i;" here (yes, different than rng, more obvious (at
least to me) an
[libvirt] [PATCH] qemu: Support to detach redirdev device
Attaching redirdev device has been supported for a while, but detaching
is not never implemented.
Simple procedure to test:
% lsusb
Bus 001 Device 014: ID 0781:5567 SanDisk Corp. Cruzer Blade
% usbredirserver -p 4000 0781:5567
% virsh attach-device test usb.xml
% cat usb.xml
% virsh detach-device test usb.xml
% virsh qemu-monitor-command test --pretty '{"execute": "query-chardev"}' |
grep 4000
On success, the chardev should not seen in output of above command.
---
src/conf/domain_conf.c | 67 +
src/conf/domain_conf.h | 4 ++
src/libvirt_private.syms | 3 ++
src/qemu/qemu_driver.c | 4 +-
src/qemu/qemu_hotplug.c | 97 +++-
src/qemu/qemu_hotplug.h | 3 ++
6 files changed, 176 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3b15cb4..d304232 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13825,6 +13825,73 @@ virDomainMemoryRemove(virDomainDefPtr def,
return ret;
}
+ssize_t
+virDomainRedirdevFind(virDomainDefPtr def,
+ virDomainRedirdevDefPtr redirdev)
+{
+size_t i;
+
+for (i = 0; i < def->nredirdevs; i++) {
+virDomainRedirdevDefPtr tmp = def->redirdevs[i];
+
+if (redirdev->bus != tmp->bus)
+continue;
+
+virDomainChrSourceDef source_chr = redirdev->source.chr;
+virDomainChrSourceDef tmp_chr = tmp->source.chr;
+
+if (source_chr.type != tmp_chr.type)
+continue;
+
+switch (source_chr.type) {
+case VIR_DOMAIN_CHR_TYPE_TCP:
+if (STRNEQ_NULLABLE(source_chr.data.tcp.host,
+tmp_chr.data.tcp.host))
+continue;
+if (STRNEQ_NULLABLE(source_chr.data.tcp.service,
+tmp_chr.data.tcp.service))
+continue;
+if (source_chr.data.tcp.listen != tmp_chr.data.tcp.listen)
+continue;
+if (source_chr.data.tcp.protocol != tmp_chr.data.tcp.protocol)
+continue;
+break;
+
+case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
+if (source_chr.data.spicevmc != tmp_chr.data.spicevmc)
+continue;
+break;
+
+default:
+/* Unlikely, currently redirdev only supports character device of
+ * type "tcp" and "spicevmc".
+ */
+break;
+}
+
+if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+!virDomainDeviceInfoAddressIsEqual(>info, >info))
+continue;
+
+break;
+}
+
+if (i < def->nredirdevs)
+return i;
+
+return -1;
+}
+
+virDomainRedirdevDefPtr
+virDomainRedirdevRemove(virDomainDefPtr def,
+size_t idx)
+{
+virDomainRedirdevDefPtr ret = def->redirdevs[idx];
+
+VIR_DELETE_ELEMENT(def->redirdevs, idx, def->nredirdevs);
+
+return ret;
+}
char *
virDomainDefGetDefaultEmulator(virDomainDefPtr def,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1de3be3..03c0155 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2538,6 +2538,10 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr
def);
void virDomainHostdevDefFree(virDomainHostdevDefPtr def);
void virDomainHubDefFree(virDomainHubDefPtr def);
void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def);
+ssize_t virDomainRedirdevFind(virDomainDefPtr def,
+ virDomainRedirdevDefPtr redirdev);
+virDomainRedirdevDefPtr virDomainRedirdevRemove(virDomainDefPtr def,
+size_t idx);
void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def);
void virDomainShmemDefFree(virDomainShmemDefPtr def);
void virDomainDeviceDefFree(virDomainDeviceDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 4b40612..ad7d82c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -423,6 +423,9 @@ virDomainPMSuspendedReasonTypeFromString;
virDomainPMSuspendedReasonTypeToString;
virDomainRedirdevBusTypeFromString;
virDomainRedirdevBusTypeToString;
+virDomainRedirdevDefFree;
+virDomainRedirdevFind;
+virDomainRedirdevRemove;
virDomainRNGBackendTypeToString;
virDomainRNGDefFree;
virDomainRNGFind;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 45ff3c0..8905af6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7736,6 +7736,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
case VIR_DOMAIN_DEVICE_MEMORY:
ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory);
break;
+case VIR_DOMAIN_DEVICE_REDIRDEV:
+ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev);
+break;
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
@@ -7748,7 +7751,6 @@
Re: [libvirt] [PATCH V2] Fix bug of attaching redirdev device
On 2016年02月25日 16:18, Michal Privoznik wrote:
On 22.02.2016 18:41, Michal Privoznik wrote:
On 22.02.2016 17:44, Osier Yang wrote:
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1298070
The corresponding chardev must be attached first, otherwise the
the qemu command line won't be complete (missing the host part),
---
src/qemu/qemu_hotplug.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ee305e7..40cead7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1381,6 +1381,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDefPtr def = vm->def;
+char *charAlias = NULL;
char *devstr = NULL;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
@@ -1391,6 +1392,10 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
if (qemuAssignDeviceRedirdevAlias(vm->def, redirdev, -1) < 0)
goto error;
+
+if (virAsprintf(, "char%s", redirdev->info.alias) < 0)
+goto error;
+
if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
goto error;
@@ -1398,6 +1403,14 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
goto error;
qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorAttachCharDev(priv->mon,
+ charAlias,
+ &(redirdev->source.chr)) < 0) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto error;
+}
+VIR_FREE(charAlias);
+
ret = qemuMonitorAddDevice(priv->mon, devstr);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -1414,9 +1427,9 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
return 0;
error:
+VIR_FREE(charAlias);
VIR_FREE(devstr);
return -1;
-
}
static int
ACK with this squashed in:
I went ahead, squashed that in and pushed because we are getting close
to the release and it would be nice to have this bugfix in it.
Thanks, I have no time to post another patch yet, it's to implement
redirdev detaching, will post tonight.
Regards,
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH V2] Fix bug of attaching redirdev device
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1298070
The corresponding chardev must be attached first, otherwise the
the qemu command line won't be complete (missing the host part),
---
src/qemu/qemu_hotplug.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ee305e7..40cead7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1381,6 +1381,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDefPtr def = vm->def;
+char *charAlias = NULL;
char *devstr = NULL;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
@@ -1391,6 +1392,10 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
if (qemuAssignDeviceRedirdevAlias(vm->def, redirdev, -1) < 0)
goto error;
+
+if (virAsprintf(, "char%s", redirdev->info.alias) < 0)
+goto error;
+
if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
goto error;
@@ -1398,6 +1403,14 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
goto error;
qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorAttachCharDev(priv->mon,
+ charAlias,
+ &(redirdev->source.chr)) < 0) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto error;
+}
+VIR_FREE(charAlias);
+
ret = qemuMonitorAddDevice(priv->mon, devstr);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -1414,9 +1427,9 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
return 0;
error:
+VIR_FREE(charAlias);
VIR_FREE(devstr);
return -1;
-
}
static int
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug of attaching redirdev device
On 2016年02月23日 00:21, Michal Privoznik wrote:
On 18.02.2016 17:02, Osier Yang wrote:
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1298070
Hey! It's nice to see people returning :)
Have been about two years! :-)
The corresponding chardev must be attached first, otherwise the
the qemu command line won't be complete (missing the host part),
---
src/qemu/qemu_hotplug.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ee305e7..5095e31 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1381,6 +1381,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDefPtr def = vm->def;
+char *charAlias = NULL;
char *devstr = NULL;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
@@ -1391,21 +1392,33 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
if (qemuAssignDeviceRedirdevAlias(vm->def, redirdev, -1) < 0)
goto error;
-if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
-goto error;
-if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0)
+if (virAsprintf(, "char%s", redirdev->info.alias) < 0)
goto error;
qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorAttachCharDev(priv->mon,
+ charAlias,
+ &(redirdev->source.chr)) < 0) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto error;
+}
+VIR_FREE(charAlias);
+
+if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
+goto rollback;
+
+if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0)
+goto rollback;
+
ret = qemuMonitorAddDevice(priv->mon, devstr);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto error;
+goto rollback;
This usually means that domain died meanwhile, so there's no need for
performing rollback. It will fail anyway.
Reasonable.
virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0);
if (ret < 0)
-goto error;
+goto rollback;
This would end up calling a monitor command without corresponding
Enter/ExitMonitor guards.
Oops, yes.
Moreover, I think the time spent in monitor should be the shortest
possible. Therefore all the preparation (e.g. allocation) should be done
upfront, before entering monitor. I think I see a way how to achieve
that. Would you mind rewriting that? If rewritten wisely no rollback
label should be needed.
V2 posted. Thanks.
Regards,
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix bug of attaching redirdev device
ping?
> >> On 18 Feb 2016, at 16:02, Osier Yang <os...@yunify.com> wrote:
> >>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1298070
> >>>
> >>> The corresponding chardev must be attached first, otherwise the
> >>> the qemu command line won't be complete (missing the host part),
> >>> ---
> >>> src/qemu/qemu_hotplug.c | 27 ++-
> >>> 1 file changed, 22 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> >>> index ee305e7..5095e31 100644
> >>> --- a/src/qemu/qemu_hotplug.c
> >>> +++ b/src/qemu/qemu_hotplug.c
> >>> @@ -1381,6 +1381,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
> >>> driver,
> >>> int ret;
> >>> qemuDomainObjPrivatePtr priv = vm->privateData;
> >>> virDomainDefPtr def = vm->def;
> >>> +char *charAlias = NULL;
> >>> char *devstr = NULL;
> >>>
> >>> if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> >>> @@ -1391,21 +1392,33 @@ int
> >>> qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
> >>>
> >>> if (qemuAssignDeviceRedirdevAlias(vm->def, redirdev, -1) < 0)
> >>> goto error;
> >>> -if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev,
> >>> priv->qemuCaps)))
> >>> -goto error;
> >>>
> >>> -if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0)
> >>> +if (virAsprintf(, "char%s", redirdev->info.alias) < 0)
> >>> goto error;
> >>>
> >>> qemuDomainObjEnterMonitor(driver, vm);
> >>> +if (qemuMonitorAttachCharDev(priv->mon,
> >>> + charAlias,
> >>> + &(redirdev->source.chr)) < 0) {
> >>> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> >>> +goto error;
> >>> +}
> >>> +VIR_FREE(charAlias);
> >>> +
> >>> +if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev,
> >>> priv->qemuCaps)))
> >>> +goto rollback;
> >>> +
> >>> +if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0)
> >>> +goto rollback;
> >>> +
> >>> ret = qemuMonitorAddDevice(priv->mon, devstr);
> >>>
> >>> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> >>> -goto error;
> >>> +goto rollback;
> >>>
> >>> virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0);
> >>> if (ret < 0)
> >>> -goto error;
> >>> +goto rollback;
> >>>
> >>> vm->def->redirdevs[vm->def->nredirdevs++] = redirdev;
> >>>
> >>> @@ -1414,9 +1427,13 @@ int
> >>> qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
> >>> return 0;
> >>>
> >>> error:
> >>> +VIR_FREE(charAlias);
> >>> VIR_FREE(devstr);
> >>> return -1;
> >>>
> >>> + rollback:
> >>> +ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
> >>> +goto error;
> >>> }
> >>>
> >>> static int
> >>> --
> >>> 2.1.4
> >>>
> >>> --
> >>> libvir-list mailing list
> >>> libvir-list@redhat.com
> >>> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix bug of attaching redirdev device
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1298070
The corresponding chardev must be attached first, otherwise the
the qemu command line won't be complete (missing the host part),
---
src/qemu/qemu_hotplug.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ee305e7..5095e31 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1381,6 +1381,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
int ret;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDefPtr def = vm->def;
+char *charAlias = NULL;
char *devstr = NULL;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
@@ -1391,21 +1392,33 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
if (qemuAssignDeviceRedirdevAlias(vm->def, redirdev, -1) < 0)
goto error;
-if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
-goto error;
-if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0)
+if (virAsprintf(, "char%s", redirdev->info.alias) < 0)
goto error;
qemuDomainObjEnterMonitor(driver, vm);
+if (qemuMonitorAttachCharDev(priv->mon,
+ charAlias,
+ &(redirdev->source.chr)) < 0) {
+ignore_value(qemuDomainObjExitMonitor(driver, vm));
+goto error;
+}
+VIR_FREE(charAlias);
+
+if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps)))
+goto rollback;
+
+if (VIR_REALLOC_N(vm->def->redirdevs, vm->def->nredirdevs+1) < 0)
+goto rollback;
+
ret = qemuMonitorAddDevice(priv->mon, devstr);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto error;
+goto rollback;
virDomainAuditRedirdev(vm, redirdev, "attach", ret == 0);
if (ret < 0)
-goto error;
+goto rollback;
vm->def->redirdevs[vm->def->nredirdevs++] = redirdev;
@@ -1414,9 +1427,13 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
driver,
return 0;
error:
+VIR_FREE(charAlias);
VIR_FREE(devstr);
return -1;
+ rollback:
+ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
+goto error;
}
static int
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Forbid sgio support for SCSI generic host device
On 13/03/14 15:50, Jiri Denemark wrote:
On Wed, Mar 12, 2014 at 23:13:24 +0800, Osier Yang wrote:
On 12/03/14 21:54, Jiri Denemark wrote:
On Fri, Mar 07, 2014 at 22:23:26 +0800, Osier Yang wrote:
The kernel didn't support the unprivileged SGIO for SCSI generic
device finally, and since it's unknow whether the way to support
unprivileged SGIO for SCSI generic device will be similar as for
SCSI block device or not, even it's simliar (I.e. via sysfs, for
SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio,
for example), the file name might be different, So it's better not
guess what it should be like currently.
This patch removes the related code (mainly about the shareable
checking on the sgio setting, it's not supported at all, why
we leave checking code there? :-), and error out if sgio is
specified in the domain config.
---
src/qemu/qemu_conf.c | 87
1 file changed, 20 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2c397b0..ad6348d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
...
@@ -1135,22 +1102,15 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
} else if (dev-type == VIR_DOMAIN_DEVICE_HOSTDEV) {
hostdev = dev-data.hostdev;
-if (!hostdev-shareable ||
-!(hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
- hostdev-source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI))
-return 0;
In the follow-up patch, you recovered just the
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI
part. Shouldn't the other checks in this condition remain?
-
-if (!(hostdev_name = virSCSIDeviceGetDevName(NULL,
-
hostdev-source.subsys.u.scsi.adapter,
-
hostdev-source.subsys.u.scsi.bus,
-
hostdev-source.subsys.u.scsi.target,
-
hostdev-source.subsys.u.scsi.unit)))
-goto cleanup;
-
-if (virAsprintf(hostdev_path, /dev/%s, hostdev_name) 0)
+if (hostdev-source.subsys.u.scsi.sgio) {
In other words, should this be something like
if (hostdev-shareable
hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI
hostdev-source.subsys.u.scsi.sgio) {
Should be:
If (hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI
hostdev-source.subsys.u.scsi.sgio) {
Regardless of whether the SCSI generic device is shareable or not. We
won't support
the unpiv_sgio setting. I lost the SUBSYS checking in the follow up
patch indeed. If no
other issue, I can squash it in when pushing. Thanks for the reviewing.
OK, makes sense. ACK with this change then.
Thanks, pushed.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Forbid sgio support for SCSI generic host device
On 12/03/14 21:54, Jiri Denemark wrote:
On Fri, Mar 07, 2014 at 22:23:26 +0800, Osier Yang wrote:
The kernel didn't support the unprivileged SGIO for SCSI generic
device finally, and since it's unknow whether the way to support
unprivileged SGIO for SCSI generic device will be similar as for
SCSI block device or not, even it's simliar (I.e. via sysfs, for
SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio,
for example), the file name might be different, So it's better not
guess what it should be like currently.
This patch removes the related code (mainly about the shareable
checking on the sgio setting, it's not supported at all, why
we leave checking code there? :-), and error out if sgio is
specified in the domain config.
---
src/qemu/qemu_conf.c | 87
1 file changed, 20 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2c397b0..ad6348d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
...
@@ -1135,22 +1102,15 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
} else if (dev-type == VIR_DOMAIN_DEVICE_HOSTDEV) {
hostdev = dev-data.hostdev;
-if (!hostdev-shareable ||
-!(hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
- hostdev-source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI))
-return 0;
In the follow-up patch, you recovered just the
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI
part. Shouldn't the other checks in this condition remain?
-
-if (!(hostdev_name = virSCSIDeviceGetDevName(NULL,
-
hostdev-source.subsys.u.scsi.adapter,
-
hostdev-source.subsys.u.scsi.bus,
-
hostdev-source.subsys.u.scsi.target,
-
hostdev-source.subsys.u.scsi.unit)))
-goto cleanup;
-
-if (virAsprintf(hostdev_path, /dev/%s, hostdev_name) 0)
+if (hostdev-source.subsys.u.scsi.sgio) {
In other words, should this be something like
if (hostdev-shareable
hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI
hostdev-source.subsys.u.scsi.sgio) {
Should be:
If (hostdev-mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS
hostdev-source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI
hostdev-source.subsys.u.scsi.sgio) {
Regardless of whether the SCSI generic device is shareable or not. We
won't support
the unpiv_sgio setting. I lost the SUBSYS checking in the follow up
patch indeed. If no
other issue, I can squash it in when pushing. Thanks for the reviewing.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Forbid sgio support for SCSI generic host device
On 07/03/14 22:23, Osier Yang wrote:
The kernel didn't support the unprivileged SGIO for SCSI generic
device finally, and since it's unknow whether the way to support
unprivileged SGIO for SCSI generic device will be similar as for
SCSI block device or not, even it's simliar (I.e. via sysfs, for
SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio,
for example), the file name might be different, So it's better not
guess what it should be like currently.
This patch removes the related code (mainly about the shareable
checking on the sgio setting, it's not supported at all, why
we leave checking code there? :-), and error out if sgio is
specified in the domain config.
---
src/qemu/qemu_conf.c | 87
1 file changed, 20 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2c397b0..ad6348d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -739,12 +739,8 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
virDomainDeviceDefPtr dev)
{
virDomainDiskDefPtr disk = NULL;
-virDomainHostdevDefPtr hostdev = NULL;
char *sysfs_path = NULL;
char *key = NULL;
-char *hostdev_name = NULL;
-char *hostdev_path = NULL;
-char *device_path = NULL;
int val;
int ret = 0;
@@ -756,27 +752,11 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
*/
if (disk-device != VIR_DOMAIN_DISK_DEVICE_LUN)
return 0;
-
-device_path = disk-src;
-} else if (dev-type == VIR_DOMAIN_DEVICE_HOSTDEV) {
-hostdev = dev-data.hostdev;
-
-if (!(hostdev_name = virSCSIDeviceGetDevName(NULL,
-
hostdev-source.subsys.u.scsi.adapter,
-
hostdev-source.subsys.u.scsi.bus,
-
hostdev-source.subsys.u.scsi.target,
-
hostdev-source.subsys.u.scsi.unit)))
-goto cleanup;
-
-if (virAsprintf(hostdev_path, /dev/%s, hostdev_name) 0)
-goto cleanup;
-
-device_path = hostdev_path;
} else {
return 0;
}
-if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL))) {
+if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk-src, NULL))) {
ret = -1;
goto cleanup;
}
@@ -787,7 +767,7 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
if (!virFileExists(sysfs_path))
goto cleanup;
-if (!(key = qemuGetSharedDeviceKey(device_path))) {
+if (!(key = qemuGetSharedDeviceKey(disk-src))) {
ret = -1;
goto cleanup;
}
@@ -798,7 +778,7 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
if (!(virHashLookup(sharedDevices, key)))
goto cleanup;
-if (virGetDeviceUnprivSGIO(device_path, NULL, val) 0) {
+if (virGetDeviceUnprivSGIO(disk-src, NULL, val) 0) {
ret = -1;
goto cleanup;
}
@@ -810,36 +790,25 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
disk-sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED))
goto cleanup;
-if (dev-type == VIR_DOMAIN_DEVICE_DISK) {
-if (disk-type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(sgio of shared disk 'pool=%s' 'volume=%s' conflicts
- with other active domains),
- disk-srcpool-pool,
- disk-srcpool-volume);
-} else {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(sgio of shared disk '%s' conflicts with other
- active domains), disk-src);
-}
+if (disk-type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _(sgio of shared disk 'pool=%s' 'volume=%s' conflicts
+ with other active domains),
+ disk-srcpool-pool,
+ disk-srcpool-volume);
} else {
virReportError(VIR_ERR_OPERATION_INVALID,
- _(sgio of shared scsi host device '%s-%d-%d-%d' conflicts
- with other active domains),
- hostdev-source.subsys.u.scsi.adapter,
- hostdev-source.subsys.u.scsi.bus,
- hostdev-source.subsys.u.scsi.target,
- hostdev-source.subsys.u.scsi.unit);
+ _(sgio of shared disk '%s' conflicts with other
+ active domains), disk-src);
}
ret = -1;
cleanup:
-VIR_FREE(hostdev_name);
-VIR_FREE(hostdev_path);
VIR_FREE(sysfs_path);
VIR_FREE(key);
return ret;
}
+
bool
Re: [libvirt] [PATCH] libvirt-tck: Ignore SIGPIPE in 051-daemon-hook.t
On 07/03/14 23:46, Mike Latimer wrote:
On Friday, March 07, 2014 05:16:48 PM Osier Yang wrote:
$hook-cleanup();
+
+# Restarting libvirtd broke the tck connection, so ignore sigpipe and
+# undefine $tck to avoid a return code of 141
+$SIG{PIPE} = 'IGNORE';
+undef $tck;
We should get the libvirt connection closed before restarting
libvirtd, in tck, it should be $tck-cleanup().
I should have stated in the original email that $tck-cleanup() does not
help here. With just $tck-cleanup() after $hook-cleanup(), the test
still exists with a return code of 141. If I add the code to ignore the
SIGPIPE, the test ends with the following messages:
libvirt error code: 38, message: Cannot write data: Broken pipe
libvirt error code: 1, message: internal error: client socket is closed
If the test restarts libvirtd (using `service libvirtd restart`), the only way
I can get it to end with an exit code of 0 is to undefine $tck.
Hm, $tck-cleanup() doesn't close the connection, it just destroy and
undefine the existing domains, networks, and pools.
snip
sub reset {
my $self = shift;
my $conn = shift || $self-conn;
$self-reset_domains($conn);
$self-reset_networks($conn);
$self-reset_storage_pools($conn);
}
sub cleanup {
my $self = shift;
foreach my $conn (@{$self-{conns}}) {
$self-reset($conn);
}
delete $self-{conns};
}
/snip
So it looks like we need a new helper to close the connection.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-tck: Update hook syntax for libvirt 0.9.0+
On 07/03/14 00:27, Mike Latimer wrote:
Starting with libvirt 0.9.0+, hook scripts can be called from several new
locations. These locations must also be reflected in the expected logs of
the hook tests.
The final test in 052-domain-hook.t intentionally produces a failed start,
which should show the first stage in the startup hook process, followed by
all the stages in the destroy process.
In addition to the above changes, libvirtd init scripts can return daemon
status in several different ways (due to distribution differences,
sysvinit vs. systemd, etc). This patch allows for 'running|active', and
'stopped|unused|inactive' responses which the hook tests rely on.
---
lib/Sys/Virt/TCK/Hooks.pm | 17 -
scripts/hooks/051-daemon-hook.t | 2 +-
scripts/hooks/052-domain-hook.t | 6 +++---
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/lib/Sys/Virt/TCK/Hooks.pm b/lib/Sys/Virt/TCK/Hooks.pm
index 75f98e9..2ae2c2a 100644
--- a/lib/Sys/Virt/TCK/Hooks.pm
+++ b/lib/Sys/Virt/TCK/Hooks.pm
@@ -73,10 +73,10 @@ sub libvirtd_status {
my $status = `service libvirtd status`;
my $_ = $status;
-if (/running/) {
-$self-{libvirtd_status} = 'running';
-} elsif (/stopped/) {
+if (/stopped|unused|inactive/) {
$self-{libvirtd_status} = 'stopped';
+} elsif (/running|active/) {
+$self-{libvirtd_status} = 'running';
}
return $self;
@@ -146,13 +146,20 @@ sub expect_log {
} elsif ($self-{type} eq 'qemu' or $self-{type} eq 'lxc') {
if ($domain_state eq Sys::Virt::Domain::STATE_RUNNING) {
if ($action eq 'destroy') {
- $expect_log = $hook $domain_name stopped end -;
+$expect_log = $hook $domain_name stopped end -\n.
+ $hook $domain_name release end -;
} else {
die hooks testing doesn't support $action running domain;
}
} elsif ($domain_state eq Sys::Virt::Domain::STATE_SHUTOFF) {
if ($action eq 'start') {
- $expect_log = $hook $domain_name start begin -;
+$expect_log = $hook $domain_name prepare begin -\n.
+ $hook $domain_name start begin -\n.
+ $hook $domain_name started begin -;
+} elsif ($action eq 'failstart') {
+$expect_log = $hook $domain_name prepare begin -\n.
+ $hook $domain_name stopped end -\n.
+ $hook $domain_name release end -;
} else {
die hooks testing doesn't support $action shutoff domain;
}
diff --git a/scripts/hooks/051-daemon-hook.t b/scripts/hooks/051-daemon-hook.t
index d9cfb3a..165cf4e 100644
--- a/scripts/hooks/051-daemon-hook.t
+++ b/scripts/hooks/051-daemon-hook.t
@@ -85,7 +85,7 @@ SKIP: {
ok($hook-compare_log(), $hook-{name} is invoked correctly while
$hook-{action} libvirtd);
diag check if libvirtd is stopped;
-ok(`service libvirtd status` =~ /stopped/, libvirtd is stopped);
+ok(`service libvirtd status` =~ /stopped|unused|inactive/, libvirtd is
stopped);
# start libvirtd
$hook-action('start');
diff --git a/scripts/hooks/052-domain-hook.t b/scripts/hooks/052-domain-hook.t
index e3b55ec..90b8a48 100644
--- a/scripts/hooks/052-domain-hook.t
+++ b/scripts/hooks/052-domain-hook.t
@@ -90,7 +90,7 @@ SKIP: {
ok(-f $hook-{name}, $hook-{name} is invoked);
my $actual_log_data = slurp($hook-{log_name});
-diag acutal log: $hook-{log_name} '$actual_log_data';
+diag actual log: $hook-{log_name} '$actual_log_data';
diag expect log:\n $hook-{expect_log};
@@ -147,7 +147,7 @@ SKIP: {
$hook-domain_name($domain_name);
$hook-domain_state($domain_state);
-$hook-action('start');
+$hook-action('failstart');
$hook-expect_log();
diag start $domain_name;
@@ -170,7 +170,7 @@ SKIP: {
diag expect log:\n $hook-{expect_log};
diag check if the actual log is same with expected log;
-ok($hook-compare_log, $hook-{name} is invoked correctly while start
$domain_name);
+ok($hook-compare_log, $hook-{name} is invoked correctly while failing to
start $domain_name);
# undefine domain
diag undefine $domain_name;
ACK, but wondering why we didn't discover it, according to the log for
the hook scripts have been changed.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-tck: Ignore SIGPIPE in 051-daemon-hook.t
On 07/03/14 01:11, Mike Latimer wrote:
This test completes successfully, but results in a return code of 141 due to
a broken pipe when restarting libvirtd. This patch just masks the SIGPIPE
and undefines $tck to avoid the 141 return code. If there is a way to
reestablish the tck connection after the restart, that would be a better
solution.
---
scripts/hooks/051-daemon-hook.t | 5 +
1 file changed, 5 insertions(+)
diff --git a/scripts/hooks/051-daemon-hook.t b/scripts/hooks/051-daemon-hook.t
index 165cf4e..a6c3d03 100644
--- a/scripts/hooks/051-daemon-hook.t
+++ b/scripts/hooks/051-daemon-hook.t
@@ -163,5 +163,10 @@ SKIP: {
ok(`service libvirtd status` =~ /running/, libvirtd is running);
$hook-cleanup();
+
+# Restarting libvirtd broke the tck connection, so ignore sigpipe and
+# undefine $tck to avoid a return code of 141
+$SIG{PIPE} = 'IGNORE';
+undef $tck;
We should get the libvirt connection closed before restarting
libvirtd, in tck, it should be $tck-cleanup().
Regards,
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirt-tck: prefer kvm if multiple domain types exist
On 07/03/14 01:39, Mike Latimer wrote:
When matching capabilities of a guest, if multiple domain types exist (for
example, 'qemu' and 'kvm') the order in which they are returned can change.
To avoid unpredictable test results, this patch prefers kvm if that domain
type exists. If not, the behavior matches what existed before, and the first
domain type is returned.
---
lib/Sys/Virt/TCK.pm | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index b2c16e7..56547c6 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -502,7 +502,17 @@ sub match_kernel {
my @domains = $caps-guest_domain_types($i);
next unless int(@domains);
- return ($domains[0],
+# Prefer kvm if multiple domain types are returned
+my $domain;
+if (int(@domains) gt 1) {
+for (my $j = 0 ; $j int(@domains) ; $j++) {
I would use grep instead of iterating over all the elements:
$domain = kvm if (grep /^kvm$/, @domains);
+$domain = kvm if ($domains[$j] eq kvm);
+}
+}
+# If kvm was not found, default to the first one
+$domain = $domains[0] if (!defined($domain));
+
+return ($domain,
$caps-guest_domain_emulator($i, $domains[0]),
Indention problem.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Forbid sgio support for SCSI generic host device
The kernel didn't support the unprivileged SGIO for SCSI generic
device finally, and since it's unknow whether the way to support
unprivileged SGIO for SCSI generic device will be similar as for
SCSI block device or not, even it's simliar (I.e. via sysfs, for
SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio,
for example), the file name might be different, So it's better not
guess what it should be like currently.
This patch removes the related code (mainly about the shareable
checking on the sgio setting, it's not supported at all, why
we leave checking code there? :-), and error out if sgio is
specified in the domain config.
---
src/qemu/qemu_conf.c | 87
1 file changed, 20 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 2c397b0..ad6348d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -739,12 +739,8 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
virDomainDeviceDefPtr dev)
{
virDomainDiskDefPtr disk = NULL;
-virDomainHostdevDefPtr hostdev = NULL;
char *sysfs_path = NULL;
char *key = NULL;
-char *hostdev_name = NULL;
-char *hostdev_path = NULL;
-char *device_path = NULL;
int val;
int ret = 0;
@@ -756,27 +752,11 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
*/
if (disk-device != VIR_DOMAIN_DISK_DEVICE_LUN)
return 0;
-
-device_path = disk-src;
-} else if (dev-type == VIR_DOMAIN_DEVICE_HOSTDEV) {
-hostdev = dev-data.hostdev;
-
-if (!(hostdev_name = virSCSIDeviceGetDevName(NULL,
-
hostdev-source.subsys.u.scsi.adapter,
-
hostdev-source.subsys.u.scsi.bus,
-
hostdev-source.subsys.u.scsi.target,
-
hostdev-source.subsys.u.scsi.unit)))
-goto cleanup;
-
-if (virAsprintf(hostdev_path, /dev/%s, hostdev_name) 0)
-goto cleanup;
-
-device_path = hostdev_path;
} else {
return 0;
}
-if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL))) {
+if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk-src, NULL))) {
ret = -1;
goto cleanup;
}
@@ -787,7 +767,7 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
if (!virFileExists(sysfs_path))
goto cleanup;
-if (!(key = qemuGetSharedDeviceKey(device_path))) {
+if (!(key = qemuGetSharedDeviceKey(disk-src))) {
ret = -1;
goto cleanup;
}
@@ -798,7 +778,7 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
if (!(virHashLookup(sharedDevices, key)))
goto cleanup;
-if (virGetDeviceUnprivSGIO(device_path, NULL, val) 0) {
+if (virGetDeviceUnprivSGIO(disk-src, NULL, val) 0) {
ret = -1;
goto cleanup;
}
@@ -810,36 +790,25 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
disk-sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED))
goto cleanup;
-if (dev-type == VIR_DOMAIN_DEVICE_DISK) {
-if (disk-type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(sgio of shared disk 'pool=%s' 'volume=%s'
conflicts
- with other active domains),
- disk-srcpool-pool,
- disk-srcpool-volume);
-} else {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(sgio of shared disk '%s' conflicts with other
- active domains), disk-src);
-}
+if (disk-type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _(sgio of shared disk 'pool=%s' 'volume=%s' conflicts
+ with other active domains),
+ disk-srcpool-pool,
+ disk-srcpool-volume);
} else {
virReportError(VIR_ERR_OPERATION_INVALID,
- _(sgio of shared scsi host device '%s-%d-%d-%d'
conflicts
- with other active domains),
- hostdev-source.subsys.u.scsi.adapter,
- hostdev-source.subsys.u.scsi.bus,
- hostdev-source.subsys.u.scsi.target,
- hostdev-source.subsys.u.scsi.unit);
+ _(sgio of shared disk '%s' conflicts with other
+ active domains), disk-src);
}
ret = -1;
cleanup:
-VIR_FREE(hostdev_name);
-VIR_FREE(hostdev_path);
VIR_FREE(sysfs_path);
VIR_FREE(key);
return ret;
}
+
bool
qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry,
const char
Re: [libvirt] [PATCH] virscsi: Introduce virSCSIDeviceUsedByInfoFree
On 07/03/14 22:55, John Ferlan wrote:
This resolves a Coverity RESOURCE_LEAK issue introduced by commit
id 'de6fa535' where the virSCSIDeviceSetUsedBy() didn't VIR_FREE
the 'copy' or possibly VIR_STRDUP()'d values.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/util/virscsi.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 69eae24..66e3161 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -268,6 +268,14 @@ cleanup:
return ret;
}
+static void
+virSCSIDeviceUsedByInfoFree(virUsedByInfoPtr used_by)
+{
+VIR_FREE(used_by-drvname);
+VIR_FREE(used_by-domname);
+VIR_FREE(used_by);
+}
+
void
virSCSIDeviceFree(virSCSIDevicePtr dev)
{
@@ -279,11 +287,8 @@ virSCSIDeviceFree(virSCSIDevicePtr dev)
VIR_FREE(dev-id);
VIR_FREE(dev-name);
VIR_FREE(dev-sg_path);
-for (i = 0; i dev-n_used_by; i++) {
-VIR_FREE(dev-used_by[i]-drvname);
-VIR_FREE(dev-used_by[i]-domname);
-VIR_FREE(dev-used_by[i]);
-}
+for (i = 0; i dev-n_used_by; i++)
+virSCSIDeviceUsedByInfoFree(dev-used_by[i]);
VIR_FREE(dev-used_by);
VIR_FREE(dev);
}
@@ -296,10 +301,11 @@ virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
virUsedByInfoPtr copy;
if (VIR_ALLOC(copy) 0)
return -1;
-if (VIR_STRDUP(copy-drvname, drvname) 0)
-return -1;
-if (VIR_STRDUP(copy-domname, domname) 0)
+if (VIR_STRDUP(copy-drvname, drvname) 0 ||
+VIR_STRDUP(copy-domname, domname) 0) {
+virSCSIDeviceUsedByInfoFree(copy);
return -1;
+}
return VIR_APPEND_ELEMENT(dev-used_by, dev-n_used_by, copy);
}
@@ -449,9 +455,7 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list,
if (STREQ_NULLABLE(dev-used_by[i]-drvname, drvname)
STREQ_NULLABLE(dev-used_by[i]-domname, domname)) {
if (dev-n_used_by 1) {
-VIR_FREE(dev-used_by[i]-drvname);
-VIR_FREE(dev-used_by[i]-domname);
-VIR_FREE(dev-used_by[i]);
+virSCSIDeviceUsedByInfoFree(dev-used_by[i]);
VIR_DELETE_ELEMENT(dev-used_by, i, dev-n_used_by);
} else {
tmp = virSCSIDeviceListSteal(list, dev);
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Fix the build failure on s390
On 11/02/14 18:21, Martin Kletzander wrote: On Tue, Feb 11, 2014 at 11:10:16AM +0800, Osier Yang wrote: On 11/02/14 00:48, Eric Blake wrote: On 02/10/2014 06:35 AM, Osier Yang wrote: The build works fine on other architectures with commit 0b4f76fc5, but for s390: TEST: virscsitest 1) test1 ... OK 2) test2 ... libvirt: error : SCSI device '1:0:0:0': could not access /builddir/build/BUILD/libvirt-1.1.1/tests/virscsidata/sg8: No such file or directory FAILED It's caused by the patch on the s390 system either doesn't create the empty files, or removed them after the patch was applied. Anyway, this patch is to fix it by simply adding useless numbers to the 2 test input files. --- tests/virscsidata/sg0 | 1 + tests/virscsidata/sg8 | 1 + 2 files changed, 2 insertions(+) Why are we modifying upstream? This sounds like a downstream issue with patch application, so downstream should come up with alternative ways to create empty files into existence when applying patches, without modifying the content of the empty file upstream. Hacking the way of applying the patch works for downstream, but I don't think it's guraranteed same problem must not happen for upstream release. IIUC, there is no way why this should not work upstream. Therefore if any downstream has problems with back-porting such patches, they should make sure their patch usage works with such patches, for example by using 'patch -E' in building scripts, '%patch -E' in spec-file, etc. Okay, I'm fine with hacking downstream, with hoping the patch set for shareable SCSI host device won't be backported much. Btw, patch -E removes the empty files after the patch is applied, this is what I'm trying to avoid, the right way is to use patch --posix. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: Fix the build failure on s390
The build works fine on other architectures with commit 0b4f76fc5, but for s390: TEST: virscsitest 1) test1 ... OK 2) test2 ... libvirt: error : SCSI device '1:0:0:0': could not access /builddir/build/BUILD/libvirt-1.1.1/tests/virscsidata/sg8: No such file or directory FAILED It's caused by the patch on the s390 system either doesn't create the empty files, or removed them after the patch was applied. Anyway, this patch is to fix it by simply adding useless numbers to the 2 test input files. --- tests/virscsidata/sg0 | 1 + tests/virscsidata/sg8 | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/virscsidata/sg0 b/tests/virscsidata/sg0 index e69de29..573541a 100644 --- a/tests/virscsidata/sg0 +++ b/tests/virscsidata/sg0 @@ -0,0 +1 @@ +0 diff --git a/tests/virscsidata/sg8 b/tests/virscsidata/sg8 index e69de29..45a4fb7 100644 --- a/tests/virscsidata/sg8 +++ b/tests/virscsidata/sg8 @@ -0,0 +1 @@ +8 -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Fix the build failure on s390
On 10/02/14 21:48, Jiri Denemark wrote: On Mon, Feb 10, 2014 at 21:35:18 +0800, Osier Yang wrote: The build works fine on other architectures with commit 0b4f76fc5, but for s390: TEST: virscsitest 1) test1 ... OK 2) test2 ... libvirt: error : SCSI device '1:0:0:0': could not access /builddir/build/BUILD/libvirt-1.1.1/tests/virscsidata/sg8: No such file or directory FAILED It's caused by the patch on the s390 system either doesn't create the empty files, or removed them after the patch was applied. Anyway, this patch is to fix it by simply adding useless numbers to the 2 test input files. This is pretty strange. AFAIK no patch binary creates empty files although git does that. If the build failed just because of that, it should have failed on other archs too. It's depended on the version of *patch*, for example, the attached patch creates two files, file aaa is not empty, bbb is empty: % cat aaa Hello, World! % cat bbb *On Fedora 19:* % patch -p1 0001-Funny-empty-files.patch patching file aaa % ls aaa bbb ls: cannot access bbb: No such file or directory aaa % patch --version | head -1 patch 2.6.1 *On RHEL7.0:* % patch -p1 0001-Funny-empty-files.patch patching file aaa patching file bbb % ls aaa bbb aaa bbb % patch --version | head -1 GNU patch 2.7.1 Note that on Fedora 19, the output of the patch command only says patching file aaa, for the empty file bbb, nothing was happened, this is what exactly I saw in the build.log from your scratch build (the failed one, sorry for that, btw). Osier From d9fda88c0d32484a349737d97926aeb34de40d81 Mon Sep 17 00:00:00 2001 From: Osier Yang jy...@redhat.com Date: Mon, 10 Feb 2014 07:16:04 -0700 Subject: [PATCH] Funny empty files --- aaa | 1 + bbb | 0 2 files changed, 1 insertion(+) create mode 100644 aaa create mode 100644 bbb diff --git a/aaa b/aaa new file mode 100644 index 000..8ab686e --- /dev/null +++ b/aaa @@ -0,0 +1 @@ +Hello, World! diff --git a/bbb b/bbb new file mode 100644 index 000..e69de29 -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Fix the build failure on s390
On 10/02/14 22:31, Osier Yang wrote: On 10/02/14 21:48, Jiri Denemark wrote: On Mon, Feb 10, 2014 at 21:35:18 +0800, Osier Yang wrote: The build works fine on other architectures with commit 0b4f76fc5, but for s390: TEST: virscsitest 1) test1 ... OK 2) test2 ... libvirt: error : SCSI device '1:0:0:0': could not access /builddir/build/BUILD/libvirt-1.1.1/tests/virscsidata/sg8: No such file or directory FAILED It's caused by the patch on the s390 system either doesn't create the empty files, or removed them after the patch was applied. Anyway, this patch is to fix it by simply adding useless numbers to the 2 test input files. This is pretty strange. AFAIK no patch binary creates empty files although git does that. If the build failed just because of that, it should have failed on other archs too. It's depended on the version of *patch*, for example, the attached patch creates two files, file aaa is not empty, bbb is empty: % cat aaa Hello, World! % cat bbb *On Fedora 19:* % patch -p1 0001-Funny-empty-files.patch patching file aaa % ls aaa bbb ls: cannot access bbb: No such file or directory aaa % patch --version | head -1 patch 2.6.1 *On RHEL7.0:* % patch -p1 0001-Funny-empty-files.patch patching file aaa patching file bbb % ls aaa bbb aaa bbb % patch --version | head -1 GNU patch 2.7.1 Note that on Fedora 19, the output of the patch command only says patching file aaa, for the empty file bbb, nothing was happened, this is what exactly I saw in the build.log from your scratch build (the failed one, sorry for that, btw). We have other empty empty files too (the test input files, the only ones which are empty in the source, except ChangeLog and AUTHORS): % find tests -type f -empty tests/fchostdata/fc_host/host4/vport_delete tests/fchostdata/fc_host/host4/vport_create tests/fchostdata/fc_host/host5/vport_delete tests/fchostdata/fc_host/host5/vport_create tests/qemuxml2argvdata/qemuxml2argv-pseries-vio-address-clash.args tests/xencapsdata/xen-ppc64.cpuinfo tests/qemuhelpdata/qemu-kvm-0.12.3-device tests/qemuhelpdata/qemu-0.12.1-device But since all of above are not created by the *.patch, instead, they are in the tarball (libvirt-$version.tar.gz), so the problem was hidden Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: Fix the build failure on s390
On 11/02/14 00:48, Eric Blake wrote: On 02/10/2014 06:35 AM, Osier Yang wrote: The build works fine on other architectures with commit 0b4f76fc5, but for s390: TEST: virscsitest 1) test1 ... OK 2) test2 ... libvirt: error : SCSI device '1:0:0:0': could not access /builddir/build/BUILD/libvirt-1.1.1/tests/virscsidata/sg8: No such file or directory FAILED It's caused by the patch on the s390 system either doesn't create the empty files, or removed them after the patch was applied. Anyway, this patch is to fix it by simply adding useless numbers to the 2 test input files. --- tests/virscsidata/sg0 | 1 + tests/virscsidata/sg8 | 1 + 2 files changed, 2 insertions(+) Why are we modifying upstream? This sounds like a downstream issue with patch application, so downstream should come up with alternative ways to create empty files into existence when applying patches, without modifying the content of the empty file upstream. Hacking the way of applying the patch works for downstream, but I don't think it's guraranteed same problem must not happen for upstream release. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Fix the error message for scsi host device's shareable checking
This fixes the wrong argument order. --- Pushed under trivial rule. --- src/qemu/qemu_hostdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 2b11d6c..1b16386 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -1135,8 +1135,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, virReportError(VIR_ERR_OPERATION_INVALID, _(SCSI device %s is already in use by other domain(s) as '%s'), - tmp_shareable ? shareable : non-shareable, - virSCSIDeviceGetName(tmp)); + virSCSIDeviceGetName(tmp), + tmp_shareable ? shareable : non-shareable); goto error; } -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix the error message for scsi host device's shareable checking
On 30/01/14 18:22, Jiri Denemark wrote:
On Thu, Jan 30, 2014 at 16:58:18 +0800, Osier Yang wrote:
This fixes the wrong argument order.
---
Pushed under trivial rule.
---
src/qemu/qemu_hostdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 2b11d6c..1b16386 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -1135,8 +1135,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
virReportError(VIR_ERR_OPERATION_INVALID,
_(SCSI device %s is already in use by
other domain(s) as '%s'),
- tmp_shareable ? shareable : non-shareable,
- virSCSIDeviceGetName(tmp));
+ virSCSIDeviceGetName(tmp),
+ tmp_shareable ? shareable : non-shareable);
goto error;
}
While this fixes wrong argument order, it is still wrong because it's
untranslatable. The code should be rewritten as
if (tmp_shareable) {
virReportError(VIR_ERR_OPERATION_INVALID,
_(SCSI device shareable is already in use by
other domain(s) as '%s'),
virSCSIDeviceGetName(tmp));
} else {
virReportError(VIR_ERR_OPERATION_INVALID,
_(SCSI device non-shareable is already in use by
other domain(s) as '%s'),
virSCSIDeviceGetName(tmp));
}
I thought whether to translate the two strings (shareable and
non-shareable)
is trivial, so choosed the conditional operator instead of if...else.
But if you keep
thinking it should be changed, I will do.
Not to mention that the error message itself doesn't make a lot of sense
to me... Did you wanted to say something else, e.g.:
if (scsi_shareable !tmp_shareable) {
virReportError(VIR_ERR_OPERATION_INVALID,
_(Shareable SCSI device '%s' is already in
use by other domain(s) as non-shareable
device '%s'),
virSCSIDeviceGetName(scsi),
virSCSIDeviceGetName(tmp));
} else if (!scsi_shareable) {
virReportError(VIR_ERR_OPERATION_INVALID,
_(Non-shareable SCSI device '%s' is already in
use by other domain(s) as device '%s'),
virSCSIDeviceGetName(scsi),
virSCSIDeviceGetName(tmp));
}
With this, the error message will be like:
Shareable SCSI device '1:0:0:0' is already in use by other domain(s) as
non-shareable device '1:0:0:0'
IMO it's a bit redundant. And I'm not sure if in some cases the SCSI
device itself just
cannot be shared, so personaly I'd like not use words like Shareable
SCSI device,
which may introduce confusion.
Thanks.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Fix the scsi util test failure
Detected by Pavel reported. Osier Yang (2): util: Accept test data path for scsi device's sg_path tests: Modify the scsi util tests src/util/virscsi.c | 3 ++- tests/virscsidata/1:0:0:0/block/sdh/dev| 1 + tests/virscsidata/1:0:0:0/block/sr0/dev| 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev | 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev | 1 + tests/virscsidata/sg0 | 0 tests/virscsidata/sg8 | 0 tests/virscsitest.c| 4 ++-- 8 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 tests/virscsidata/1:0:0:0/block/sdh/dev delete mode 100644 tests/virscsidata/1:0:0:0/block/sr0/dev delete mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev create mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev create mode 100644 tests/virscsidata/sg0 create mode 100644 tests/virscsidata/sg8 -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] tests: Modify the scsi util tests
Add tests/virscsidata/sg0 and tests/virscsidata/sg8 as the test input for constructing scsi-sg_path. And change the scsi generic number of 1:0:0:0, because it's easy to hide the problem (assuming most machines have a CDROM drive). Signed-off-by: Osier Yang jy...@redhat.com --- tests/virscsidata/1:0:0:0/block/sdh/dev| 1 + tests/virscsidata/1:0:0:0/block/sr0/dev| 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev | 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev | 1 + tests/virscsidata/sg0 | 0 tests/virscsidata/sg8 | 0 tests/virscsitest.c| 4 ++-- 7 files changed, 4 insertions(+), 4 deletions(-) create mode 100644 tests/virscsidata/1:0:0:0/block/sdh/dev delete mode 100644 tests/virscsidata/1:0:0:0/block/sr0/dev delete mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev create mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev create mode 100644 tests/virscsidata/sg0 create mode 100644 tests/virscsidata/sg8 diff --git a/tests/virscsidata/1:0:0:0/block/sdh/dev b/tests/virscsidata/1:0:0:0/block/sdh/dev new file mode 100644 index 000..3d33f0f --- /dev/null +++ b/tests/virscsidata/1:0:0:0/block/sdh/dev @@ -0,0 +1 @@ +11:0 diff --git a/tests/virscsidata/1:0:0:0/block/sr0/dev b/tests/virscsidata/1:0:0:0/block/sr0/dev deleted file mode 100644 index 3d33f0f..000 --- a/tests/virscsidata/1:0:0:0/block/sr0/dev +++ /dev/null @@ -1 +0,0 @@ -11:0 diff --git a/tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev b/tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev deleted file mode 100644 index bd84814..000 --- a/tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev +++ /dev/null @@ -1 +0,0 @@ -21:1 diff --git a/tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev b/tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev new file mode 100644 index 000..bd84814 --- /dev/null +++ b/tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev @@ -0,0 +1 @@ +21:1 diff --git a/tests/virscsidata/sg0 b/tests/virscsidata/sg0 new file mode 100644 index 000..e69de29 diff --git a/tests/virscsidata/sg8 b/tests/virscsidata/sg8 new file mode 100644 index 000..e69de29 diff --git a/tests/virscsitest.c b/tests/virscsitest.c index d256a0e..d4b3e4a 100644 --- a/tests/virscsitest.c +++ b/tests/virscsitest.c @@ -43,7 +43,7 @@ test1(const void *data ATTRIBUTE_UNUSED) scsi_host1, 0, 0, 0))) return -1; -if (STRNEQ(name, sr0)) +if (STRNEQ(name, sdh)) goto cleanup; ret = 0; @@ -72,7 +72,7 @@ test2(const void *data ATTRIBUTE_UNUSED) sgname = virSCSIDeviceGetSgName(virscsi_prefix, scsi_host1, 0, 0, 0); -if (!sgname || STRNEQ(sgname, sg1)) +if (!sgname || STRNEQ(sgname, sg8)) goto cleanup; if (!(dev = virSCSIDeviceNew(virscsi_prefix, scsi_host1, -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: Accept test data path for scsi device's sg_path
Commit 10c9ceff6d intended to introduce new argument for the
testing purpose, but it missed the similar changing of the
device's sg_path. The problem was hidden since my laptop has
the /dev/sg0 and /dev/sg1. A later patch will modify the tests
accordingly.
Signed-off-by: Osier Yang jy...@redhat.com
Reported-by: Hrdina Pavel phrd...@redhat.com
---
src/util/virscsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 731a01c..acc3815 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -222,7 +222,8 @@ virSCSIDeviceNew(const char *sysfs_prefix,
if (virAsprintf(dev-name, %d:%d:%d:%d, dev-adapter,
dev-bus, dev-target, dev-unit) 0 ||
-virAsprintf(dev-sg_path, /dev/%s, sg) 0)
+virAsprintf(dev-sg_path, %s/%s,
+sysfs_prefix ? sysfs_prefix : /dev, sg) 0)
goto cleanup;
if (!virFileExists(dev-sg_path)) {
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix the scsi util test failure
On 30/01/14 19:48, Osier Yang wrote: Detected by Pavel reported. Sorry, s/Pavel reported/Hrdina Pavel/, Osier Yang (2): util: Accept test data path for scsi device's sg_path tests: Modify the scsi util tests src/util/virscsi.c | 3 ++- tests/virscsidata/1:0:0:0/block/sdh/dev| 1 + tests/virscsidata/1:0:0:0/block/sr0/dev| 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev | 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev | 1 + tests/virscsidata/sg0 | 0 tests/virscsidata/sg8 | 0 tests/virscsitest.c| 4 ++-- 8 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 tests/virscsidata/1:0:0:0/block/sdh/dev delete mode 100644 tests/virscsidata/1:0:0:0/block/sr0/dev delete mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev create mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev create mode 100644 tests/virscsidata/sg0 create mode 100644 tests/virscsidata/sg8 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix the scsi util test failure
On 30/01/14 23:37, Pavel Hrdina wrote: On 30.1.2014 14:20, Pavel Hrdina wrote: On 30.1.2014 12:48, Osier Yang wrote: Detected by Pavel reported. Osier Yang (2): util: Accept test data path for scsi device's sg_path tests: Modify the scsi util tests src/util/virscsi.c | 3 ++- tests/virscsidata/1:0:0:0/block/sdh/dev| 1 + tests/virscsidata/1:0:0:0/block/sr0/dev| 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev | 1 - tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev | 1 + tests/virscsidata/sg0 | 0 tests/virscsidata/sg8 | 0 tests/virscsitest.c| 4 ++-- 8 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 tests/virscsidata/1:0:0:0/block/sdh/dev delete mode 100644 tests/virscsidata/1:0:0:0/block/sr0/dev delete mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg1/dev create mode 100644 tests/virscsidata/1:0:0:0/scsi_generic/sg8/dev create mode 100644 tests/virscsidata/sg0 create mode 100644 tests/virscsidata/sg8 ACK series, the test now works also on RHEL-7. Pavel I've pushed the patches, Thanks. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] qemu: Don't fail if the SCSI host device is shareable between domains
It doesn't make sense to fail if the SCSI host device is specified
as shareable explicitly between domains (NB, it works if and only
if the device is specified as shareable for *all* domains,
otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable). And the change on the data struct brings on many
subsequent changes in the code.
Since prior to this patch, the shareable tag didn't work as expected,
it actually work like non-shareable. If there was a live domain using
the SCSI device with shareable specified, then after upgrading
(assuming the live domain keep running during upgrading) the older
libvirt (without this patch) to newer libvirt (with this patch), it may
cause some confusion when user tries to start later domains as
shareable. So this patch also added notes in formatdomain.html to
declare the fact.
* src/util/virscsi.h:
- Remove virSCSIDeviceGetUsedBy
- Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
- Add virSCSIDeviceIsAvailable
* src/util/virscsi.c:
- struct virSCSIDevice: Change used_by to be an array; Add
n_used_by as the array count
- virSCSIDeviceGetUsedBy: Removed
- virSCSIDeviceFree: frees the used_by array
- virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
memory corruption
- virSCSIDeviceIsAvailable: New
- virSCSIDeviceListDel: Change the logic, for device which is already
in the list, just remove the corresponding entry in used_by. And
since it's only used in one place, we can safely removing the code
to find out the dev in the list first.
- Copyright updating
* src/libvirt_private.sys:
- virSCSIDeviceGetUsedBy: Remove
- virSCSIDeviceIsAvailable: New
* src/qemu/qemu_hostdev.c:
- qemuUpdateActiveScsiHostdevs: Check if the device existing before
adding it to the list;
- qemuPrepareHostdevSCSIDevices: Error out if the not all domains
use the device as shareable; Also don't try to add the device
to the activeScsiHostdevs list if it already there; And make
more sensible error w.r.t the current shareable value in
driver-activeScsiHostdevs.
- qemuDomainReAttachHostScsiDevices: Change the logic according
to the changes on helpers.
Signed-off-by: Osier Yang jy...@redhat.com
---
docs/formatdomain.html.in | 5 +++
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 77 ++-
src/util/virscsi.c| 44 +--
src/util/virscsi.h| 7 +++--
5 files changed, 88 insertions(+), 47 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index ff50214..20b3f5a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2798,6 +2798,11 @@
between domains (assuming the hypervisor and OS support this).
Only supported by SCSI host device.
span class=sinceSince 1.0.6/span
+p
+ Note: Although codeshareable/code was introduced
+ span class=sincein 1.0.6/span, but it did not work as
+ as expected until span class=since1.2.2/span.
+/p
/dd
/dl
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0c16343..4dbba17 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1687,7 +1687,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..2b9d274 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+virSCSIDevicePtr scsi = NULL;
+virSCSIDevicePtr tmp = NULL;
if (!def-nhostdevs)
return 0;
virObjectLock(driver-activeScsiHostdevs);
for (i = 0; i def-nhostdevs; i++) {
-virSCSIDevicePtr scsi = NULL;
hostdev = def-hostdevs[i];
if (hostdev-mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev-shareable)))
goto cleanup;
-virSCSIDeviceSetUsedBy(scsi, def-name);
-
-if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0) {
+if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
+if (virSCSIDeviceSetUsedBy(tmp, def-name) 0) {
+virSCSIDeviceFree(scsi);
+goto cleanup;
+}
virSCSIDeviceFree(scsi);
-goto cleanup;
+} else {
+if (virSCSIDeviceSetUsedBy(scsi
[libvirt] [PATCH 2/2] util: Add tests for scsi utils
To support to pass the path of the test data to the utils, one
more argument is added to virSCSIDeviceGetSgName,
virSCSIDeviceGetDevName, and virSCSIDeviceNew, and the related
code is changed accordingly.
Signed-off-by: Osier Yang jy...@redhat.com
---
src/qemu/qemu_cgroup.c | 3 +-
src/qemu/qemu_command.c | 3 +-
src/qemu/qemu_command.h | 3 +-
src/qemu/qemu_conf.c | 12 ++-
src/qemu/qemu_hostdev.c | 9 +-
src/security/security_apparmor.c | 3 +-
src/security/security_dac.c | 6 +-
src/security/security_selinux.c | 6 +-
src/util/virscsi.c | 26 +++---
src/util/virscsi.h | 9 +-
tests/Makefile.am| 14 +++
tests/testutilsqemu.c| 3 +-
tests/virscsitest.c | 188 +++
13 files changed, 256 insertions(+), 29 deletions(-)
create mode 100644 tests/virscsitest.c
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index de20f2d..a97f184 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -291,7 +291,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
-if ((scsi = virSCSIDeviceNew(dev-source.subsys.u.scsi.adapter,
+if ((scsi = virSCSIDeviceNew(NULL,
+ dev-source.subsys.u.scsi.adapter,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 96b8825..e1ff287 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5785,7 +5785,8 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
virBuffer buf = VIR_BUFFER_INITIALIZER;
char *sg = NULL;
-sg =
(callbacks-qemuGetSCSIDeviceSgName)(dev-source.subsys.u.scsi.adapter,
+sg = (callbacks-qemuGetSCSIDeviceSgName)(NULL,
+
dev-source.subsys.u.scsi.adapter,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit);
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index de7683d..82ca4b3 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -57,7 +57,8 @@
typedef struct _qemuBuildCommandLineCallbacks qemuBuildCommandLineCallbacks;
typedef qemuBuildCommandLineCallbacks *qemuBuildCommandLineCallbacksPtr;
struct _qemuBuildCommandLineCallbacks {
-char * (*qemuGetSCSIDeviceSgName) (const char *adapter,
+char * (*qemuGetSCSIDeviceSgName) (const char *sysfs_prefix,
+ const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit);
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index ac53f6d..0c246c0 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -761,7 +761,8 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices,
} else if (dev-type == VIR_DOMAIN_DEVICE_HOSTDEV) {
hostdev = dev-data.hostdev;
-if (!(hostdev_name =
virSCSIDeviceGetDevName(hostdev-source.subsys.u.scsi.adapter,
+if (!(hostdev_name = virSCSIDeviceGetDevName(NULL,
+
hostdev-source.subsys.u.scsi.adapter,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit)))
@@ -949,7 +950,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
if (!(key = qemuGetSharedDeviceKey(disk-src)))
goto cleanup;
} else {
-if (!(dev_name =
virSCSIDeviceGetDevName(hostdev-source.subsys.u.scsi.adapter,
+if (!(dev_name = virSCSIDeviceGetDevName(NULL,
+
hostdev-source.subsys.u.scsi.adapter,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit)))
@@ -1053,7 +1055,8 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver,
if (!(key = qemuGetSharedDeviceKey(disk-src)))
goto cleanup;
} else {
-if (!(dev_name =
virSCSIDeviceGetDevName(hostdev-source.subsys.u.scsi.adapter,
+if (!(dev_name = virSCSIDeviceGetDevName(NULL,
+
hostdev
[libvirt] [PATCH 0/2 v4] qemu: Don't fail if the SCSI host device is shareable between domains
*** BLURB HERE *** Osier Yang (2): qemu: Don't fail if the SCSI host device is shareable between domains tests: Add tests for scsi utils docs/formatdomain.html.in| 5 ++ src/libvirt_private.syms | 2 +- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 12 ++- src/qemu/qemu_hostdev.c | 86 ++ src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 +- src/security/security_selinux.c | 6 +- src/util/virscsi.c | 70 +++ src/util/virscsi.h | 16 ++-- tests/Makefile.am| 14 +++ tests/testutilsqemu.c| 3 +- tests/virscsitest.c | 188 +++ 15 files changed, 344 insertions(+), 76 deletions(-) create mode 100644 tests/virscsitest.c -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 30/01/14 03:31, Eric Blake wrote: On 01/29/2014 10:22 AM, Osier Yang wrote: It doesn't make sense to fail if the SCSI host device is specified as shareable explicitly between domains (NB, it works if and only if the device is specified as shareable for *all* domains, otherwise it fails). To fix the problem, this patch introduces an array for virSCSIDevice struct, which records all the names of domain which are using the device (note that the recorded domains must specifiy the device as s/specifiy/specify/ shareable). And the change on the data struct brings on many subsequent changes in the code. Since prior to this patch, the shareable tag didn't work as expected, it actually work like non-shareable. If there was a live domain using the SCSI device with shareable specified, then after upgrading (assuming the live domain keep running during upgrading) the older libvirt (without this patch) to newer libvirt (with this patch), it may cause some confusion when user tries to start later domains as shareable. So this patch also added notes in formatdomain.html to declare the fact. I'm not sure I follow what you're trying to warn about here - when you upgrade libvirtd, doesn't the state reloading figure out which domains have a device in use, as part of the reload machinery? That is, while the old code mishandled shareable with regards to which domain is using the device, we don't actually store the array of domains using a device in XML, but recompute it on libvirtd restart. Thus, upgrading to a newer libvirtd (whether release 1.2.2 or to a version with this patch backported), then starting yet another guest with a shareable request, should work correctly, right? I think both John and I were confused and forgot upgrading will triger configuration reloading. :-) Will reword the commit log a bit when pushing. Thanks. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] util: Add tests for scsi utils
On 30/01/14 03:35, Eric Blake wrote: On 01/29/2014 10:22 AM, Osier Yang wrote: To support to pass the path of the test data to the utils, one s/to pass/passing/ Changed. more argument is added to virSCSIDeviceGetSgName, virSCSIDeviceGetDevName, and virSCSIDeviceNew, and the related code is changed accordingly. Signed-off-by: Osier Yang jy...@redhat.com --- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 12 ++- src/qemu/qemu_hostdev.c | 9 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 +- src/security/security_selinux.c | 6 +- src/util/virscsi.c | 26 +++--- src/util/virscsi.h | 9 +- tests/Makefile.am| 14 +++ tests/testutilsqemu.c| 3 +- tests/virscsitest.c | 188 +++ It might be worth splitting this into two patches - one that adds the argument and adjusts existing callers, and the other that adds the new testsuite addition. Splitted and added virscsitest to .gitignore. Pushed. Thanks. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] storage: Fix autostart of pool with fc_host type adapter
On 22/01/14 21:36, John Ferlan wrote: On 01/07/2014 06:07 AM, Osier Yang wrote: On 07/01/14 01:21, John Ferlan wrote: On 01/06/2014 05:19 AM, Osier Yang wrote: The checkPool is a bit different for pool with fc_host type source adapter, since the vHBA it's based on might be not created yet (it's created by startPool, which is involked after checkPool in storageDriverAutostart). So it should not fail, otherwise the autostart of the pool will fail either. The problem is easy to reproduce: * Enable autostart for the pool * Restart libvirtd service * Check the pool's state --- src/storage/storage_backend_scsi.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) Not sure this is the right thing to do. With this change it doesn't matter what getAdapterName() returns for fc_host's... It matters with if *isActive is true or false in the end. We don't need to try to start the pool if it's already active. Fair enough; however, let's consider the failure case. On failure, a message is reported and name == NULL. Do we need to clear that error now? I think my original thoughts were along the lines of having getAdapterName do more of the heavy lifting. Have both check and start call it. It would call the createVport which would be mostly unchanged, except for the virFileWaitForDevices() which could move to start... Found your method *might* break one logic. Assuming the pool is not marked as autostart, and the vHBA was not created yet. Since with your method checkPool will create the vHBA now, then it's possible for checkPool to return *isActive as true, as long as the device path for the created vHBA can show up in host very quickly (like what we talked a lot in PATCH 2/2, how long it will show up is also depended, the time can be long, but it also can be short), i.e. during the checkPool process. And thus the pool will be marked as Active. As the result, the problem on one side of the coin is fixed, but it introduces a similar problem on another side. :-) That's why I said previously I don't want to do any creation work in checkPool, it should only check something. So, I'm going forward to push my patch, with the last error reset in checkPool. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: Add shareable field for virSCSIDevice struct
On 16/01/14 08:51, John Ferlan wrote:
On 01/08/2014 09:51 AM, Osier Yang wrote:
...
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 751eaf0..3998c3a 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -58,6 +58,7 @@ struct _virSCSIDevice {
const char *used_by; /* name of the domain using this dev */
bool readonly;
+bool shareable;
};
struct _virSCSIDeviceList {
@@ -185,7 +186,8 @@ virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly)
+ bool readonly,
+ bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
char *sg = NULL;
@@ -201,6 +203,7 @@ virSCSIDeviceNew(const char *adapter,
dev-target = target;
dev-unit = unit;
dev-readonly = readonly;
+dev-shareable= shareable;
You still didn't add the space here before the =
ACK if you do. I don't believe this is 1.2.1 material.
This patch is standalone. Pushed with the indention fixed.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Build failed in Jenkins: libvirt-syntax-check #1898
On 23/01/14 18:11, Jenkins CI wrote: See http://honk.sigxcpu.org:8001/job/libvirt-syntax-check/1898/ -- Started by upstream project libvirt-build build number 2132 Started by upstream project libvirt-build build number 2133 Building on master in workspace http://honk.sigxcpu.org:8001/job/libvirt-syntax-check/ws/ [workspace] $ /bin/sh -xe /tmp/hudson9098701041827209295.sh + make syntax-check GENbracket-spacing-check src/util/virscsi.c:206: dev-shareable= shareable; Oh, no, I actually missed push the change. Pushing it. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage: Fix the memory leak
The return value of virGetFCHostNameByWWN is a strdup'ed string.
Also add comments to declare that the caller should take care of
freeing it.
---
src/storage/storage_backend_scsi.c | 15 ++-
src/util/virutil.c | 5 -
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index fce2bae..b38e530 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -668,6 +668,8 @@ static int
deleteVport(virStoragePoolSourceAdapter adapter)
{
unsigned int parent_host;
+char *name = NULL;
+int ret = -1;
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
return 0;
@@ -680,18 +682,21 @@ deleteVport(virStoragePoolSourceAdapter adapter)
if (!adapter.data.fchost.parent)
return 0;
-if (!(virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
-adapter.data.fchost.wwpn)))
+if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
+ adapter.data.fchost.wwpn)))
return -1;
if (getHostNumber(adapter.data.fchost.parent, parent_host) 0)
-return -1;
+goto cleanup;
if (virManageVport(parent_host, adapter.data.fchost.wwpn,
adapter.data.fchost.wwnn, VPORT_DELETE) 0)
-return -1;
+goto cleanup;
-return 0;
+ret = 0;
+cleanup:
+VIR_FREE(name);
+return ret;
}
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 87cc2e7..7a2fbb0 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1820,7 +1820,10 @@ cleanup:
/* virGetHostNameByWWN:
*
* Iterate over the sysfs tree to get FC host name (e.g. host5)
- * by wwnn,wwpn pair.
+ * by the provided wwnn,wwpn pair.
+ *
+ * Returns the FC host name which must be freed by the caller,
+ * or NULL on failure.
*/
char *
virGetFCHostNameByWWN(const char *sysfs_prefix,
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Fix the memory leak
On 23/01/14 19:06, Martin Kletzander wrote: On Thu, Jan 23, 2014 at 06:23:32PM +0800, Osier Yang wrote: The return value of virGetFCHostNameByWWN is a strdup'ed string. Also add comments to declare that the caller should take care of freeing it. --- src/storage/storage_backend_scsi.c | 15 ++- src/util/virutil.c | 5 - 2 files changed, 14 insertions(+), 6 deletions(-) ACK, Thanks, pushed. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] storage: Fix autostart of pool with fc_host type adapter
On 23/01/14 21:35, John Ferlan wrote:
On 01/23/2014 04:45 AM, Osier Yang wrote:
On 22/01/14 21:36, John Ferlan wrote:
On 01/07/2014 06:07 AM, Osier Yang wrote:
On 07/01/14 01:21, John Ferlan wrote:
On 01/06/2014 05:19 AM, Osier Yang wrote:
The checkPool is a bit different for pool with fc_host
type source adapter, since the vHBA it's based on might be
not created yet (it's created by startPool, which is
involked after checkPool in storageDriverAutostart). So it
should not fail, otherwise the autostart of the pool will
fail either.
The problem is easy to reproduce:
* Enable autostart for the pool
* Restart libvirtd service
* Check the pool's state
---
src/storage/storage_backend_scsi.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
Not sure this is the right thing to do. With this change it doesn't
matter what getAdapterName() returns for fc_host's...
It matters with if *isActive is true or false in the end. We don't
need to try to start the pool if it's already active.
Fair enough; however, let's consider the failure case. On failure, a
message is reported and name == NULL. Do we need to clear that error now?
I think my original thoughts were along the lines of having
getAdapterName do more of the heavy lifting. Have both check and start
call it. It would call the createVport which would be mostly unchanged,
except for the virFileWaitForDevices() which could move to start...
Found your method *might* break one logic.
Assuming the pool is not marked as autostart, and the vHBA was not
created yet. Since with your method checkPool will create the vHBA now,
then it's possible for checkPool to return *isActive as true, as long as
the device path for the created vHBA can show up in host very quickly
(like what we talked a lot in PATCH 2/2, how long it will show up is also
depended, the time can be long, but it also can be short), i.e. during the
checkPool process. And thus the pool will be marked as Active. As
the result, the problem on one side of the coin is fixed, but it introduces
a similar problem on another side. :-)
Huh? Not sure I see what problem you're driving at.
Look back at the caller - isActive from _scsi translates to started in
the caller. The 'started' is used to decide whether to call 'startPool'
and 'refreshPool'. If autostart is disabled, then 'startPool' won't be
called.
Assuming pool-autostart if false, and started is happened to be true.
Calling refreshPool is likely to cause pool-active == 1.
...
if (started) {
if (backend-refreshPool(conn, pool) 0) {
virErrorPtr err = virGetLastError();
if (backend-stopPool)
backend-stopPool(conn, pool);
VIR_ERROR(_(Failed to autostart storage pool '%s': %s),
pool-def-name, err ? err-message :
_(no error message found));
virStoragePoolObjUnlock(pool);
continue;
}
pool-active = 1;
}
/...
Since the vHBA was already created in checkPool, then I see not much
reason why refreshPool can not be success.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH python] Fix calling of virStreamSend method
On 23/01/14 22:25, Daniel P. Berrange wrote:
About the subject prefix, [libvirt-python] should be better, like
what we did for Perl binding.
Change d40861 removed the 'len' argument from the virStreamSend
C level wrapper, but forgot to remove it from the python level
wrapper.
Reported-by: Robie Basak robie.ba...@canonical.com
Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
libvirt-override-virStream.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-override-virStream.py b/libvirt-override-virStream.py
index cd44314..ce82da6 100644
--- a/libvirt-override-virStream.py
+++ b/libvirt-override-virStream.py
@@ -122,6 +122,6 @@
with the call, but may instead be delayed until a
subsequent call.
-ret = libvirtmod.virStreamSend(self._o, data, len(data))
+ret = libvirtmod.virStreamSend(self._o, data)
if ret == -1: raise libvirtError ('virStreamSend() failed')
return ret
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3] qemu: Don't fail if the SCSI host device is shareable between domains
It doesn't make sense to fail if the SCSI host device is specified
as shareable explicitly between domains (NB, it works if and only
if the device is specified as shareable for *all* domains,
otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable). And the change on the data struct brings on many
subsequent changes in the code.
Since prior to this patch, the shareable tag didn't work as expected,
it actually work like non-shareable. If there was a live domain using
the SCSI device with shareable specified, then after upgrading
(assuming the live domain keep running during upgrading) the older
libvirt (without this patch) to newer libvirt (with this patch), it may
cause some confusion when user tries to start later domains as
shareable. So this patch also added notes in formatdomain.html to
declare the fact.
* src/util/virscsi.h:
- Remove virSCSIDeviceGetUsedBy
- Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
- Add virSCSIDeviceIsAvailable
* src/util/virscsi.c:
- struct virSCSIDevice: Change used_by to be an array; Add
n_used_by as the array count
- virSCSIDeviceGetUsedBy: Removed
- virSCSIDeviceFree: frees the used_by array
- virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
memory corruption
- virSCSIDeviceIsAvailable: New
- virSCSIDeviceListDel: Change the logic, for device which is already
in the list, just remove the corresponding entry in used_by. And
since it's only used in one place, we can safely removing the code
to find out the dev in the list first.
- Copyright updating
* src/libvirt_private.sys:
- virSCSIDeviceGetUsedBy: Remove
- virSCSIDeviceIsAvailable: New
* src/qemu/qemu_hostdev.c:
- qemuUpdateActiveScsiHostdevs: Check if the device existing before
adding it to the list;
- qemuPrepareHostdevSCSIDevices: Error out if the not all domains
use the device as shareable; Also don't try to add the device
to the activeScsiHostdevs list if it already there; And make
more sensible error w.r.t the current shareable value in
driver-activeScsiHostdevs.
- qemuDomainReAttachHostScsiDevices: Change the logic according
to the changes on helpers.
---
docs/formatdomain.html.in | 6
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 77 ++-
src/util/virscsi.c| 45 +--
src/util/virscsi.h| 7 +++--
5 files changed, 90 insertions(+), 47 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index ff50214..18bfad1 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2798,6 +2798,12 @@
between domains (assuming the hypervisor and OS support this).
Only supported by SCSI host device.
span class=sinceSince 1.0.6/span
+p
+ Note: though codeshareable/shareable was introduced
+ span class=sincesince 1.0.6/span, but it doesn't work as
+ as expected (actually works like non-shareable) until
+ span class=since1.2.2/span.
+/p
/dd
/dl
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 13caf93..0f30292 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1686,7 +1686,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..2b9d274 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+virSCSIDevicePtr scsi = NULL;
+virSCSIDevicePtr tmp = NULL;
if (!def-nhostdevs)
return 0;
virObjectLock(driver-activeScsiHostdevs);
for (i = 0; i def-nhostdevs; i++) {
-virSCSIDevicePtr scsi = NULL;
hostdev = def-hostdevs[i];
if (hostdev-mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev-shareable)))
goto cleanup;
-virSCSIDeviceSetUsedBy(scsi, def-name);
-
-if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0) {
+if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
+if (virSCSIDeviceSetUsedBy(tmp, def-name) 0) {
+virSCSIDeviceFree(scsi);
+goto cleanup;
+}
virSCSIDeviceFree(scsi);
-goto cleanup;
+} else {
+if
Re: [libvirt] [PATCH v3] qemu: Don't fail if the SCSI host device is shareable between domains
On 23/01/14 23:04, Osier Yang wrote: It doesn't make sense to fail if the SCSI host device is specified as shareable explicitly between domains (NB, it works if and only if the device is specified as shareable for *all* domains, otherwise it fails). To fix the problem, this patch introduces an array for virSCSIDevice struct, which records all the names of domain which are using the device (note that the recorded domains must specifiy the device as shareable). And the change on the data struct brings on many subsequent changes in the code. Since prior to this patch, the shareable tag didn't work as expected, it actually work like non-shareable. If there was a live domain using the SCSI device with shareable specified, then after upgrading (assuming the live domain keep running during upgrading) the older libvirt (without this patch) to newer libvirt (with this patch), it may cause some confusion when user tries to start later domains as shareable. So this patch also added notes in formatdomain.html to declare the fact. * src/util/virscsi.h: - Remove virSCSIDeviceGetUsedBy - Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel - Add virSCSIDeviceIsAvailable * src/util/virscsi.c: - struct virSCSIDevice: Change used_by to be an array; Add n_used_by as the array count - virSCSIDeviceGetUsedBy: Removed - virSCSIDeviceFree: frees the used_by array - virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential memory corruption - virSCSIDeviceIsAvailable: New - virSCSIDeviceListDel: Change the logic, for device which is already in the list, just remove the corresponding entry in used_by. And since it's only used in one place, we can safely removing the code to find out the dev in the list first. - Copyright updating * src/libvirt_private.sys: - virSCSIDeviceGetUsedBy: Remove - virSCSIDeviceIsAvailable: New * src/qemu/qemu_hostdev.c: - qemuUpdateActiveScsiHostdevs: Check if the device existing before adding it to the list; - qemuPrepareHostdevSCSIDevices: Error out if the not all domains use the device as shareable; Also don't try to add the device to the activeScsiHostdevs list if it already there; And make more sensible error w.r.t the current shareable value in driver-activeScsiHostdevs. - qemuDomainReAttachHostScsiDevices: Change the logic according to the changes on helpers. --- docs/formatdomain.html.in | 6 src/libvirt_private.syms | 2 +- src/qemu/qemu_hostdev.c | 77 ++- src/util/virscsi.c| 45 +-- src/util/virscsi.h| 7 +++-- 5 files changed, 90 insertions(+), 47 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ff50214..18bfad1 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2798,6 +2798,12 @@ between domains (assuming the hypervisor and OS support this). Only supported by SCSI host device. span class=sinceSince 1.0.6/span +p + Note: though codeshareable/shareable was introduced s/shareable/code/, :( -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Doc: Improve the document for nodesuspend
On 22/01/14 17:32, Ján Tomko wrote: On 01/22/2014 07:55 AM, Osier Yang wrote: Explicitly lists the possible values for --target option; Gets rid of the confused strings like Suspend-to-RAM; Emphasises the node *has to* be suspended in the time duration specified by --duration. And rewords the entire document a bit according to the API's implementation and document. --- tools/virsh.pod | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index 8839e3c..35a7292 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -297,11 +297,14 @@ If Icell is specified, this will prints specified cell statistics only. =item Bnodesuspend [Itarget] [Iduration] -Puts the node (host machine) into a system-wide sleep state such as -Suspend-to-RAM, Suspend-to-Disk or Hybrid-Suspend and sets up a -Real-Time-Clock interrupt to fire (to wake up the node) after a time delay -specified by the 'duration' parameter. The duration time should be -at least 60 seconds. +Puts the node (host machine) into a system-wide sleep state and schedule +the node's Real-Time-Clock interrupt to resume the node after the time +duration specified by Iduration is out. +Itarget specifies the state to which the host will be suspended to, it +can be mem(suspended to RAM), disk(suspended to disk), or hybrid +(suspended to both RAM and disk). Iduration specifies the time duration This would look better with spaces in front of the parentheses. I think 'suspend' instead of 'suspended' would look better inside the parentheses too. Pushed with above suggestions, thanks for the reviewing. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] Doc: Add note for node-memory-tune
On 22/01/14 17:33, Ján Tomko wrote: On 01/22/2014 07:55 AM, Osier Yang wrote: To let the user know the command onlys work for KSM under Linux. --- tools/virsh.pod | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/virsh.pod b/tools/virsh.pod index 35a7292..07f4397 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -317,6 +317,9 @@ different numa nodes can be merged. When set to 0, only pages which physically reside in the memory area of same NUMA node can be merged. When set to 1, pages from all nodes can be merged. Default to 1. +BNote: Currently the shared memory service only means KSM (Kernel Samepage +Merging). + =item Bcapabilities Print an XML document describing the capabilities of the hypervisor ACK If you're touching this, you should also add 'shm-merge-across-nodes' to the list of node-memory-tunes parameters a few lines above. Oh, right, I pushed with it's added. Thanks. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Avoid crash in qemuDiskGetActualType
On 22/01/14 18:18, Peter Krempa wrote:
Libvirtd would crash if a domain contained an empty cdrom drive of
type='volume' as the disk def-srcpool member would be dereferenced. Fix
it by checking if the source pool is present before dereferencing it.
Also alter tests to catch this issue in the future.
Reported by: Kevin Shanahan
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1056328
---
src/qemu/qemu_conf.c | 2 +-
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args | 2 ++
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 6 ++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4378791..ac53f6d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1302,7 +1302,7 @@ cleanup:
int
qemuDiskGetActualType(virDomainDiskDefPtr def)
{
-if (def-type == VIR_DOMAIN_DISK_TYPE_VOLUME)
+if (def-type == VIR_DOMAIN_DISK_TYPE_VOLUME def-srcpool)
return def-srcpool-actualtype;
Returning the type as volume should be fine, since there is no
case statement for volume type when building the drive's
command line, and the source is empty anyway.
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Correct the NUMA node range checking
There are 2 issues here: First we shouldn't add 1 to the return
value of numa_max_node(), since the semanteme of the error message
was changed, it's not saying about the number of total NUMA nodes
anymore. Second, the value of bit is the position of the first
bit which exceeds either numa_max_node() or NUMA_NUM_NODES, it can
be any number in the range, so saying bigger than $bit is quite
confused now. For example, assuming there is a NUMA machine which
has 10 NUMA nodes, and one specifies the nodeset as 0,5,88,
the error message will be like:
Nodeset is out of range, host cannot support NUMA node bigger than 88
It sounds like all NUMA node number less than 88 is fine, but
actually the maximum NUMA node number the machine supports is 9.
This patch fixes the issues by removing the addition with 1 and
simplifies the error message as NUMA node $bit is out of range.
---
src/util/virnuma.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index ab46591..500dca7 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -99,7 +99,6 @@ virNumaSetupMemoryPolicy(virNumaTuneDef numatune,
int ret = -1;
int bit = 0;
size_t i;
-int maxnode = 0;
virBitmapPtr tmp_nodemask = NULL;
if (numatune.memory.placement_mode ==
@@ -122,16 +121,13 @@ virNumaSetupMemoryPolicy(virNumaTuneDef numatune,
return -1;
}
-maxnode = numa_max_node() + 1;
-
/* Convert nodemask to NUMA bitmask. */
nodemask_zero(mask);
bit = -1;
while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) = 0) {
-if (bit maxnode || bit NUMA_NUM_NODES) {
+if (bit numa_max_node() || bit NUMA_NUM_NODES) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _(Nodeset is out of range, host cannot support
- NUMA node bigger than %d), bit);
+ _(NUMA node %d is out of range), bit);
return -1;
}
nodemask_set(mask, bit);
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] storage: Polling the sysfs for pool with fc_host type adapter
On 22/01/14 22:02, John Ferlan wrote: On 01/07/2014 05:37 AM, Osier Yang wrote: On 07/01/14 02:30, John Ferlan wrote: On 01/06/2014 05:19 AM, Osier Yang wrote: The SCSI device corresponding to the vHBA might not show up in sysfs yet when we trying to scan the LUNs. As a result, we will end up with an empty volume set for the pool after pool-start, even if there are LUNs. So what causes the delay to get the LUN's into sysfs? It's basicly from the delay of udev. Is there something that can be done at creation time (or wherever) to sync that sooner? I thought like that, let's say at the point of createVport. But the createVport is just to create the vHBA, and nothing else to do, the left work for device showing up in the sysfs tree is on the udev then. Polling right after createVport for the SCSI device in sysfs tree will take more time. Is there a way to determine that the SCSI device hasn't shown up yet other than the readdir()? You're adding a delay/loop for some other subsystem's inability? to sync or provide the resources. It's the only way AFAIK. Though the time of the device showing up is rather depended, better than doing nothing, this patch introduces the polling with 5 * 1 seconds in maximum (the time works fine on my testing machine at least). Note that for the pool which doesn't have any LUN, it will still take 5 seconds to poll, but it's not a bad trade, 5 seconds is not much, and in most cases, one won't use an empty pool in practice. Since the paths that call into this only seem to be via refresh and perhaps a subsequent refresh would resolve things. Exactly, in most cases it will work, since the time window between pool-start and pool-refresh should be enough for the SCSI device showing up in the sysfs tree, *generally*. but it's not necessary to be that. Could this be better served by documenting that it's possible that depending on circumstance X (answer to my first question) that in order to see elements in the pool, one may have to reload again. Assuming of course that the next reload would find them... I thought like this too, but the problem is it's not guaranteed that the volume could be loaded after execute pool-refresh one time, may be 2 times, 3 times, ... N times. Also the time of each pool-refresh is not fixed, it depends on how long the udevadm settle (see virFileWaitForDevices) will take. I guess I'm a bit cautious about adding randomly picked timeout values based on some test because while it may work for you, perhaps it's 10 seconds for someone else. While you may consider a 5 second pause OK and reasonable a customer may not consider that to be reasonable. People (and testers) do strange and random things. Exactly, this is not the only problem we faced regarding to the storage stuffs, and the users keeps asking why, why, and why. Furthermore, could it be possible that you catch things perfectly and only say 10 of 100 devices are found... But if you waited another 5 seconds the other 90 devices would show up.. I think by adding this code you end up down a slippery slope of handing fc_host devices specially... We are exactly on the same page, but the question is what the best solution we should provide? It looks ugly if we add documentation saying one should use pool-refresh after the pool is started, if the volumes are not loaded, but how many times to use the pool-refresh is depended? This patch was basicly a proposal for discussion. I didn't expect it could be committed smoothly. I'm still not convinced that the retry loop is the right way to go. We could conceivably still have a failure even after 5 retries at once per second. Also is sleep() the right call or should it be usleep()? I have this alarm (sic) going off in the back of my head about using sleep() in a threaded context. Although I do see node_device_driver.c uses it... Regardless of whether we (u)sleep() or not, if we still have a failure, then we're still left documenting the fact that for fc_host type pools there are conditions that need to be met which are out of the control of libvirt's scope of influence that would cause the pool to not be populated. The user is still left trying to refresh multiple times. And we still don't know exactly whey we're not seeing the devices. The reality is this is also true for other pools as well since it's not guaranteed that processLU() is ever called and 'retval' is always 0 (and probably could be removed since it's pointless). I gave up hacking in the code, instead, I'm making a patch to add document in virsh manual as a Note to prompt the fact. Something like this: ... +BNote: For pool which relies on remote resources, e.g. a iscsi type +pool, or a scsi type pool based on a (v)HBA, since the corresponding +devices for the volumes may not show up on the host's sysfs yet during +the pool starting process, it may need to refresh the pool (see +Bpool-refresh) to get the volumes correctly loaded. How many times needed
[libvirt] [PATCH 2/2 v2] storage: Add document for possible problem on volume detection
For pool which relies on remote resources, such as a iscsi type pool, since how long it takes to export the corresponding devices to host's sysfs is really depended, it could depend on the network connection, it also could depend on the host's udev procedures. So it's likely that the volumes are not able to be detected during pool starting process, polling the sysfs doesn't work, since we don't know how much time is best for the polling, and even worse, the volumes could still be not detected or partly not detected even after the polling. So we end up with a documentation to prompt the fact, in virsh manual. And as a small improvement, let's explicitly say no LUNs found in the debug log in that case. --- src/storage/storage_backend_scsi.c | 5 + tools/virsh.pod| 8 2 files changed, 13 insertions(+) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 93039c1..fbcb102 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -495,6 +495,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, DIR *devicedir = NULL; struct dirent *lun_dirent = NULL; char devicepattern[64]; +bool found = false; VIR_DEBUG(Discovering LUs on host %u, scanhost); @@ -516,11 +517,15 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, continue; } +found = true; VIR_DEBUG(Found LU '%s', lun_dirent-d_name); processLU(pool, scanhost, bus, target, lun); } +if (!found) +VIR_DEBUG(No LU found for pool %s, pool-def-name); + closedir(devicedir); return retval; diff --git a/tools/virsh.pod b/tools/virsh.pod index 77f9383..073465e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2650,6 +2650,14 @@ Refresh the list of volumes contained in Ipool. Start the storage Ipool, which is previously defined but inactive. +BNote: For pool which relies on remote resources, e.g. a iscsi type +pool, or a scsi type pool based on a (v)HBA, since the corresponding +devices for the volumes may not show up on the host's sysfs yet during +the pool starting process, it may need to refresh the pool (see +Bpool-refresh) to get the volumes correctly detected. How many times +needed for the refreshing is depended on the network connection and the +time the host takes to export the corresponding devices to sysfs. + =item Bpool-undefine Ipool-or-uuid Undefine the configuration for an inactive Ipool. -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] storage: Fix autostart of pool with fc_host type adapter
On 22/01/14 21:36, John Ferlan wrote:
On 01/07/2014 06:07 AM, Osier Yang wrote:
On 07/01/14 01:21, John Ferlan wrote:
On 01/06/2014 05:19 AM, Osier Yang wrote:
The checkPool is a bit different for pool with fc_host
type source adapter, since the vHBA it's based on might be
not created yet (it's created by startPool, which is
involked after checkPool in storageDriverAutostart). So it
should not fail, otherwise the autostart of the pool will
fail either.
The problem is easy to reproduce:
* Enable autostart for the pool
* Restart libvirtd service
* Check the pool's state
---
src/storage/storage_backend_scsi.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
Not sure this is the right thing to do. With this change it doesn't
matter what getAdapterName() returns for fc_host's...
It matters with if *isActive is true or false in the end. We don't
need to try to start the pool if it's already active.
Fair enough; however, let's consider the failure case. On failure, a
message is reported and name == NULL. Do we need to clear that error now?
Indeed.
I've attached a format-patch output for this logic - your call as to
whether or not you want to use it... If you keep your logic, then just
decide upon how to handle the error message... It won't be necessary for
the check case, but would be for the refresh case.
I am OK with things as they are, it just looks odd in the CheckPool
function to be special casing FC_HOST just because it's not created yet
and then to have the start function do the create anyway. It just seems
we could be smarter/better.
I agree your method is more linear, except there is no need for the error
statement in startPool (hope the indention is not broken), it will just
override the useful errors in the code path.
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index d3d14ce..f6cb820 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -744,13 +744,8 @@ virStorageBackendSCSIStartPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
{
char *name = NULL;
virStoragePoolSourceAdapter adapter = pool-def-source.adapter;
-if (!(name = getAdapterName(pool-def-source.adapter))) {
-virReportError(VIR_ERR_XML_ERROR,
- _(Failed to find SCSI host with wwnn='%s',
- wwpn='%s'), adapter.data.fchost.wwnn,
- adapter.data.fchost.wwpn);
+if (!(name = getAdapterName(pool-def-source.adapter)))
return -1;
-}
VIR_FREE(name);
virFileWaitForDevices();
return 0;
To ensure things work well, I'm going to put the patch into practice
tomorrow
on a NPIV machine. If it goes well, I will push it with above change.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Correct the NUMA node range checking
On 23/01/14 08:04, Eric Blake wrote:
On 01/22/2014 04:45 AM, Osier Yang wrote:
There are 2 issues here: First we shouldn't add 1 to the return
value of numa_max_node(), since the semanteme of the error message
s/semanteme/semantics/
was changed, it's not saying about the number of total NUMA nodes
anymore. Second, the value of bit is the position of the first
bit which exceeds either numa_max_node() or NUMA_NUM_NODES, it can
be any number in the range, so saying bigger than $bit is quite
confused now. For example, assuming there is a NUMA machine which
has 10 NUMA nodes, and one specifies the nodeset as 0,5,88,
the error message will be like:
Nodeset is out of range, host cannot support NUMA node bigger than 88
It sounds like all NUMA node number less than 88 is fine, but
actually the maximum NUMA node number the machine supports is 9.
This patch fixes the issues by removing the addition with 1 and
simplifies the error message as NUMA node $bit is out of range.
---
src/util/virnuma.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
-maxnode = numa_max_node() + 1;
-
/* Convert nodemask to NUMA bitmask. */
nodemask_zero(mask);
bit = -1;
while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) = 0) {
-if (bit maxnode || bit NUMA_NUM_NODES) {
+if (bit numa_max_node() || bit NUMA_NUM_NODES) {
This calls numa_max_node() in a loop, where we used to call it just
once. Since this is third-party code, do we know how efficient it is?
It may be smarter to cache the results once than to call every iteration
of the loop, to avoid O(n*2) behavior on large hosts.
For that matter, can't we just set the max node to the smaller of
numa_max_node() and NUMA_NUM_NODES up front, and avoid the || in the loop?
Good idea, pushed with the fix, along with a bit change in the commit log.
Thanks.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2 v2] storage: Add document for possible problem on volume detection
On 22/01/14 23:15, John Ferlan wrote: On 01/22/2014 09:39 AM, Osier Yang wrote: For pool which relies on remote resources, such as a iscsi type pool, since how long it takes to export the corresponding devices to host's sysfs is really depended, it could depend on the network connection, it also could depend on the host's udev procedures. So it's likely that the volumes are not able to be detected during pool starting process, polling the sysfs doesn't work, since we don't know how much time is best for the polling, and even worse, the volumes could still be not detected or partly not detected even after the polling. So we end up with a documentation to prompt the fact, in virsh manual. Probably some of the above is overkill since the virsh.pod repeats much of it - although having the intro comment specifically target udev procedures (or udevadm settle) as the culprit are good. Also see my change to your text below. And as a small improvement, let's explicitly say no LUNs found in the debug log in that case. --- src/storage/storage_backend_scsi.c | 5 + tools/virsh.pod| 8 2 files changed, 13 insertions(+) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 93039c1..fbcb102 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -495,6 +495,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, DIR *devicedir = NULL; struct dirent *lun_dirent = NULL; char devicepattern[64]; +bool found = false; VIR_DEBUG(Discovering LUs on host %u, scanhost); @@ -516,11 +517,15 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, continue; } +found = true; VIR_DEBUG(Found LU '%s', lun_dirent-d_name); processLU(pool, scanhost, bus, target, lun); } +if (!found) +VIR_DEBUG(No LU found for pool %s, pool-def-name); + closedir(devicedir); return retval; diff --git a/tools/virsh.pod b/tools/virsh.pod index 77f9383..073465e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2650,6 +2650,14 @@ Refresh the list of volumes contained in Ipool. Start the storage Ipool, which is previously defined but inactive. +BNote: For pool which relies on remote resources, e.g. a iscsi type +pool, or a scsi type pool based on a (v)HBA, since the corresponding +devices for the volumes may not show up on the host's sysfs yet during +the pool starting process, it may need to refresh the pool (see +Bpool-refresh) to get the volumes correctly detected. How many times +needed for the refreshing is depended on the network connection and the +time the host takes to export the corresponding devices to sysfs. + =item Bpool-undefine Ipool-or-uuid Undefine the configuration for an inactive Ipool. BNote: A storage pool that relies on remote resources such as an iscsi or a vHBA backed scsi pool may need to be refreshed multiple times in order to have all the volumes detected (see Bpool-refresh). This is because the corresponding volume devices may not be present in the host's filesystem during the initial pool startup or the current refresh attempt. The number of refresh retries is dependant upon the network connection and the time the host takes to export the corresponding devices. I like it. :-) (sic) note that formatstorage.html.in has both vHBA and (v)HBA, while We have to use (v)HBA here, since it's the same case for HBA. virsh.pod presently only uses vHBA. Whatever the *correct* syntax is The only place uses vHBA in virsh.pod is for nodedev-destroy, which only works for vHBA indeed, it doesn't work for HBA. Pushed with s/vHBA/(v)HBA/, thanks. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2 v2] storage: Add document for possible problem on volume detection
On 23/01/14 13:44, Osier Yang wrote: On 22/01/14 23:15, John Ferlan wrote: On 01/22/2014 09:39 AM, Osier Yang wrote: For pool which relies on remote resources, such as a iscsi type pool, since how long it takes to export the corresponding devices to host's sysfs is really depended, it could depend on the network connection, it also could depend on the host's udev procedures. So it's likely that the volumes are not able to be detected during pool starting process, polling the sysfs doesn't work, since we don't know how much time is best for the polling, and even worse, the volumes could still be not detected or partly not detected even after the polling. So we end up with a documentation to prompt the fact, in virsh manual. Probably some of the above is overkill since the virsh.pod repeats much of it - although having the intro comment specifically target udev procedures (or udevadm settle) as the culprit are good. Also see my change to your text below. And as a small improvement, let's explicitly say no LUNs found in the debug log in that case. --- src/storage/storage_backend_scsi.c | 5 + tools/virsh.pod| 8 2 files changed, 13 insertions(+) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 93039c1..fbcb102 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -495,6 +495,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, DIR *devicedir = NULL; struct dirent *lun_dirent = NULL; char devicepattern[64]; +bool found = false; VIR_DEBUG(Discovering LUs on host %u, scanhost); @@ -516,11 +517,15 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, continue; } +found = true; VIR_DEBUG(Found LU '%s', lun_dirent-d_name); processLU(pool, scanhost, bus, target, lun); } +if (!found) +VIR_DEBUG(No LU found for pool %s, pool-def-name); + closedir(devicedir); return retval; diff --git a/tools/virsh.pod b/tools/virsh.pod index 77f9383..073465e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2650,6 +2650,14 @@ Refresh the list of volumes contained in Ipool. Start the storage Ipool, which is previously defined but inactive. +BNote: For pool which relies on remote resources, e.g. a iscsi type +pool, or a scsi type pool based on a (v)HBA, since the corresponding +devices for the volumes may not show up on the host's sysfs yet during +the pool starting process, it may need to refresh the pool (see +Bpool-refresh) to get the volumes correctly detected. How many times +needed for the refreshing is depended on the network connection and the +time the host takes to export the corresponding devices to sysfs. + =item Bpool-undefine Ipool-or-uuid Undefine the configuration for an inactive Ipool. BNote: A storage pool that relies on remote resources such as an iscsi or a vHBA backed scsi pool may need to be refreshed multiple times in order to have all the volumes detected (see Bpool-refresh). This is because the corresponding volume devices may not be present in the host's filesystem during the initial pool startup or the current refresh attempt. The number of refresh retries is dependant upon the network connection and the time the host takes to export the corresponding devices. I like it. :-) (sic) note that formatstorage.html.in has both vHBA and (v)HBA, while We have to use (v)HBA here, since it's the same case for HBA. Btw, I'm not sure whether using (v)HBA as the abbreviation for vHBA or/and HBA is acceptable in formal document or not, if it's not quite good, it will need a further patch to clean up them in both virsh.pod and formatdomain.html. Anyway, it's something trivial. I'm going forward to push this patch. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] virsh: Fix the string breaking style
---
Pushed under trivial rule.
---
tools/virsh-host.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 967bd52..3438ae7 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -516,7 +516,7 @@ static const vshCmdInfo info_nodesuspend[] = {
},
{.name = desc,
.data = N_(Suspend the host node for a given time duration
- and attempt to resume thereafter.)
+and attempt to resume thereafter.)
},
{.name = NULL}
};
@@ -525,8 +525,8 @@ static const vshCmdOptDef opts_node_suspend[] = {
{.name = target,
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
- .help = N_(mem(Suspend-to-RAM),
-disk(Suspend-to-Disk), hybrid(Hybrid-Suspend))
+ .help = N_(mem(Suspend-to-RAM), disk(Suspend-to-Disk),
+hybrid(Hybrid-Suspend))
},
{.name = duration,
.type = VSH_OT_INT,
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] Doc fixes
*** BLURB HERE *** Osier Yang (3): virsh: Fix the string breaking style Doc: Improve the document for nodesuspend Doc: Add note for node-memory-tune tools/virsh-host.c | 6 +++--- tools/virsh.pod| 16 +++- 2 files changed, 14 insertions(+), 8 deletions(-) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] Doc: Improve the document for nodesuspend
Explicitly lists the possible values for --target option; Gets rid of the confused strings like Suspend-to-RAM; Emphasises the node *has to* be suspended in the time duration specified by --duration. And rewords the entire document a bit according to the API's implementation and document. --- tools/virsh.pod | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index 8839e3c..35a7292 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -297,11 +297,14 @@ If Icell is specified, this will prints specified cell statistics only. =item Bnodesuspend [Itarget] [Iduration] -Puts the node (host machine) into a system-wide sleep state such as -Suspend-to-RAM, Suspend-to-Disk or Hybrid-Suspend and sets up a -Real-Time-Clock interrupt to fire (to wake up the node) after a time delay -specified by the 'duration' parameter. The duration time should be -at least 60 seconds. +Puts the node (host machine) into a system-wide sleep state and schedule +the node's Real-Time-Clock interrupt to resume the node after the time +duration specified by Iduration is out. +Itarget specifies the state to which the host will be suspended to, it +can be mem(suspended to RAM), disk(suspended to disk), or hybrid +(suspended to both RAM and disk). Iduration specifies the time duration +in seconds for which the host has to be suspended, it should be at least +60 seconds. =item Bnode-memory-tune [Ishm-pages-to-scan] [Ishm-sleep-millisecs] -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] Doc: Add note for node-memory-tune
To let the user know the command onlys work for KSM under Linux. --- tools/virsh.pod | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/virsh.pod b/tools/virsh.pod index 35a7292..07f4397 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -317,6 +317,9 @@ different numa nodes can be merged. When set to 0, only pages which physically reside in the memory area of same NUMA node can be merged. When set to 1, pages from all nodes can be merged. Default to 1. +BNote: Currently the shared memory service only means KSM (Kernel Samepage +Merging). + =item Bcapabilities Print an XML document describing the capabilities of the hypervisor -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 17/01/14 21:44, John Ferlan wrote: On 01/17/2014 02:06 AM, Osier Yang wrote: ...snip... As a conclusion, I think the only concern from you is about the problem on the running domain of an old libvirt (without these 2 patches). Right? If so, my thought is to add document somewhere, though I have not much idea about where to put the document, and how to write the document to explain the problem which is such complicated. Just trying to think and reason through the possibilities to help make sure we're on the same page and reduce the chance for possible future corner case issues... With regard to the error message - just a way to indicate that the failure was due to the device not being set shareable should be fine. Whether that's this domain hasn't set it shareable or another domain hasn't set it shareable is a nice addition. Just saying already in use doesn't give enough of a hint that perhaps there is a way or for what reason we failed. Of course reading the code it's easy, but if you're a customer without code... Finally - I think a note in the Device section of formatdomain.html to indicate when support was really added. Sure the tag was added in 1.0.6, but it's not really functional until 1.2.2. Not sure how best to say that other than perhaps changing the since value... In the same area it should be noted that all domains using/defining the device need to have the shareable tag; otherwise, depending on order of domain startup one or more domains will fail to start. Agreed. formatdomain.html is a good place to do that. I will post a v3 series including the document together. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 16/01/14 08:51, John Ferlan wrote:
On 01/08/2014 09:51 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is specified
as shareable explicitly between domains (NB, it works if and only
if the device is specified as shareable for *all* domains,
otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable). And the change on the data struct brings on many
subsequent changes in the code.
* src/util/virscsi.h:
- Remove virSCSIDeviceGetUsedBy
- Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
- Add virSCSIDeviceIsAvailable
* src/util/virscsi.c:
- struct virSCSIDevice: Change used_by to be an array; Add
n_used_by as the array count
- virSCSIDeviceGetUsedBy: Removed
- virSCSIDeviceFree: frees the used_by array
- virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
memory corruption
- virSCSIDeviceIsAvailable: New
- virSCSIDeviceListDel: Change the logic, for device which is already
in the list, just remove the corresponding entry in used_by
- Copyright updating
* src/libvirt_private.sys:
- virSCSIDeviceGetUsedBy: Remove
- virSCSIDeviceIsAvailable: New
* src/qemu/qemu_hostdev.c:
- qemuUpdateActiveScsiHostdevs: Check if the device existing before
adding it to the list;
- qemuPrepareHostdevSCSIDevices: Error out if the not all domains
use the device as shareable; Also don't try to add the device
to the activeScsiHostdevs list if it already there.
- qemuDomainReAttachHostScsiDevices: Change the logic according
to the changes on helpers.
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 75 ++--
src/util/virscsi.c | 48 +--
src/util/virscsi.h | 7 +++--
4 files changed, 84 insertions(+), 48 deletions(-)
..
virObjectUnlock(driver-activeScsiHostdevs);
@@ -1380,8 +1390,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
virObjectLock(driver-activeScsiHostdevs);
for (i = 0; i nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
-virSCSIDevicePtr scsi, tmp;
-const char *used_by = NULL;
+virSCSIDevicePtr scsi;
virDomainDeviceDef dev;
dev.type = VIR_DOMAIN_DEVICE_HOSTDEV;
@@ -1411,30 +1420,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr
driver,
/* Only delete the devices which are marked as being used by @name,
* because qemuProcessStart could fail on the half way. */
-tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi);
-virSCSIDeviceFree(scsi);
-
-if (!tmp) {
+if (!virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi)) {
I think you should keep (and perhaps rename) the tmp pointer and then
pass it to virSCSIDeviceListDel() since that function will call the the
find again anyway
VIR_WARN(Unable to find device %s:%d:%d:%d
in list of active SCSI devices,
hostdev-source.subsys.u.scsi.adapter,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit);
+virSCSIDeviceFree(scsi);
continue;
}
-used_by = virSCSIDeviceGetUsedBy(tmp);
-if (STREQ_NULLABLE(used_by, name)) {
-VIR_DEBUG(Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs,
- hostdev-source.subsys.u.scsi.adapter,
- hostdev-source.subsys.u.scsi.bus,
- hostdev-source.subsys.u.scsi.target,
- hostdev-source.subsys.u.scsi.unit,
- name);
+VIR_DEBUG(Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs,
+ hostdev-source.subsys.u.scsi.adapter,
+ hostdev-source.subsys.u.scsi.bus,
+ hostdev-source.subsys.u.scsi.target,
+ hostdev-source.subsys.u.scsi.unit,
+ name);
-virSCSIDeviceListDel(driver-activeScsiHostdevs, tmp);
-}
+virSCSIDeviceListDel(driver-activeScsiHostdevs, scsi, name);
+virSCSIDeviceFree(scsi);
}
virObjectUnlock(driver-activeScsiHostdevs);
}
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 3998c3a..42030c5 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -1,6 +1,7 @@
/*
* virscsi.c: helper APIs for managing host SCSI devices
*
+ * Copyright (C) 2013 - 2014 Red Hat, Inc.
* Copyright (C) 2013 Fujitsu, Inc.
*
* This library is free software; you can redistribute it and/or
@@ -55,7 +56,8 @@ struct _virSCSIDevice {
char *name
Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
[ shrinked ]
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 75 ++--
src/util/virscsi.c | 48 +--
src/util/virscsi.h | 7 +++--
4 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65d1bde..bd5f466 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1677,7 +1677,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..9d81e94 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+virSCSIDevicePtr scsi = NULL;
+virSCSIDevicePtr tmp = NULL;
if (!def-nhostdevs)
return 0;
virObjectLock(driver-activeScsiHostdevs);
for (i = 0; i def-nhostdevs; i++) {
-virSCSIDevicePtr scsi = NULL;
hostdev = def-hostdevs[i];
if (hostdev-mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev-shareable)))
goto cleanup;
-virSCSIDeviceSetUsedBy(scsi, def-name);
-
-if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0) {
+if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
+if (virSCSIDeviceSetUsedBy(tmp, def-name) 0) {
+virSCSIDeviceFree(scsi);
+goto cleanup;
+}
virSCSIDeviceFree(scsi);
-goto cleanup;
+} else {
+if (virSCSIDeviceSetUsedBy(scsi, def-name) 0 ||
+virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0) {
+virSCSIDeviceFree(scsi);
+goto cleanup;
+}
It took some thinking, but I convinced myself that this path doesn't
need the shared check since it's only called from qemuProcessReconnect;
however, if something else did call it some day then that check may be
necessary. It may be wise to add it anyway... I have no strong opinion
about it being required for this change.
Missed reply for this one.
Actually it needs the checking. Assuming there are 2 live domains, and
they are using the same SCSI generic device. It's possible since the
device could be shared among domains. And in that case, we should
update the dev-used_by array instead of adding the device to the list
(driver-activeScsiHostdevs) directly, during the reconnecting sequence.
I.E it needs to restore the internal data correctly to the state just as the
one before libvirtd getting shutdown.
Regards,
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 16/01/14 23:39, John Ferlan wrote: On 01/16/2014 08:53 AM, Osier Yang wrote: ...snip... It took some thinking, but I convinced myself that this path doesn't need the shared check since it's only called from qemuProcessReconnect; however, if something else did call it some day then that check may be necessary. It may be wise to add it anyway... I have no strong opinion about it being required for this change. Missed reply for this one. Actually it needs the checking. Assuming there are 2 live domains, and they are using the same SCSI generic device. It's possible since the device could be shared among domains. And in that case, we should update the dev-used_by array instead of adding the device to the list (driver-activeScsiHostdevs) directly, during the reconnecting sequence. I.E it needs to restore the internal data correctly to the state just as the one before libvirtd getting shutdown. The activeScsiHostdevs is listed off the driver and not the domain, Yes. It's of the driver. so it's not like each domain has to update which other domains is sharing. But the list is filled while each domain is starting or *reconnecting* Maybe I'm misreading your thoughts regarding needing to update instead of add. If there were 2..n already running with the share attribute already set, would the reconnect threads run/finish before the new libvirtd accepts new connections/domain starts? Yes. See daemonRunStateInit. If so, then this code will restore life as it was before libvirtd was stopped. Yes. That's what this patch tries to do. The only way those domains would have been able to start previously and share the device is if they ran through the qemuPrepareHostdevSCSIDevices() which does check shareable. Yes, that's why I didn't do the checking about shareable in qemuUpdateActivePciHostdevs. My assumption was after some amount of code reading - those restart threads would finish so that it's not possible for some domain to be started that isn't sharing, but wanting to use the same device. Exactly right. Since prior code doesn't allow sharing of these devices there doesn't need to be a consideration made for an already running domain when this code first appears. Although, you may run into situations where qemuPrepareHostdevSCSIDevices() fails due to a running domain that was running before the shareable attribute was known. Indeed, but we have no way to avoid this problem for the domains which was running against the old libvirt, and the libvirt is upgraded after that. That domain would have to be restarted - something that could be documentable. The error message could be more helpful indicating it's in use without the shareable attribute by some other domain. It's more complicate than this, when a domain is trying to start, the situations could be (with new code): 1) No domain is using the device 2) 1 domain is using the device 3) More than 1 domain are using the device. For 1), it's just fine, let's ignore it. For 2), the domain which is using it could use the device as either shareable or non-shareable. If it's shareable, the later domain could be started successfully only if it uses the device as shareable too; If it's non-shareable, the later domain can't use the device anyway. For 3), the domains must use the device as shareable. And the later domain must use the device as shareable too to be started successfully. So, as you seen, we can't simply say with error the device is in use without shareable attribute by some other domain. We can use if...else branches to construct the more sensible errors, but I'm doubting about if we should make things more complicate. Though for old code, simply saying the device is in use without the shareable attribute by some other domain is fine, but if you think about the situations in new code, it will be a mess. In that case, only 1 domain would be using it thus in theory used_by[0] would be that domain. If there were more than 1 domain (n_used_by), then you've got other problems in the code! I guess here you still mean the domain(s) which were running with the old code. And since with the old code, there could be only 1 domain using the same device, regardless of the device is specified as shareable or not. So yet, the n_used_by must be 1. As a conclusion, I think the only concern from you is about the problem on the running domain of an old libvirt (without these 2 patches). Right? If so, my thought is to add document somewhere, though I have not much idea about where to put the document, and how to write the document to explain the problem which is such complicated. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/6] Fix some memory leaks and other issues find by coverity tool
On 14/01/14 01:17, John Ferlan wrote: On 01/13/2014 11:12 AM, Pavel Hrdina wrote: This patch series fixes few memory leaks found by coverity tool to make that tool happy. The last patch is adding only one comment to hide double_close error as coverity tool is wrong about this and we don't have to see it in every report. Check the patch for more information. Pavel Hrdina (6): Fix memory leak in openvz_conf.c Fix possible memory leak in phyp_driver.c Fix possible memory leak in util/virxml.c Fix possible memory leak in virsh-domain-monitor.c Fix memory leak in securityselinuxlabeltest.c Fix coverity complain in commandtest.c src/openvz/openvz_conf.c | 5 +++-- src/phyp/phyp_driver.c | 4 +++- src/util/virxml.c| 2 ++ tests/commandtest.c | 1 + tests/securityselinuxlabeltest.c | 5 - tools/virsh-domain-monitor.c | 4 6 files changed, 17 insertions(+), 4 deletions(-) hrmph... for some reason 1/6 and 4/6 haven't made it to my inbox yet. I hope the Red Hat mail filter isn't acting up again... Anyway 1/6 is an ACK 4/6 I think still issues First off 'type' and 'device' need to be initialized to NULL, then don't worry about the if details() within the (!target) condition. Second either we have to choose to exit if the virXPathString() fails for type/device or when we go to vshPrint() there needs to be a type ? type : - and device ? device : - in order to ensure we're not printing garbage or old data Both the type and device are not mandatory attributes for disk device, type defaults to file, and device defaults to disk. So in principle they can't be NULL, if virXPathString returns NULL for them, it means either the internal data structure is broken (unlikely but critical), or virXPathString itself had some funny things. For either of them, we should fail. It's unlike source element, since the disk source can be empty, e.g. for a Floppy drive. Not sure which of the methods is better if we fail to alloc type or device is better. Note that virXPathString() can return NULL for more reasons than just allocation failure. Since Osier wrote the code, maybe he can help decide the course of action to take. Is it better to display - or just fail? On an allocation failure, then virXPathString for target will probably fail too landing us in that error path, so we could just take that option too! Regards, Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2 v2] Fixes on shareable SCSI host device
*** BLURB HERE *** Osier Yang (2): util: Add shareable field for virSCSIDevice struct qemu: Don't fail if the SCSI host device is shareable between domains src/libvirt_private.syms | 3 +- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_hostdev.c | 84 ++-- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 ++- src/security/security_selinux.c | 6 ++- src/util/virscsi.c | 59 +++- src/util/virscsi.h | 11 -- 8 files changed, 116 insertions(+), 59 deletions(-) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: Add shareable field for virSCSIDevice struct
Unlike the host devices of other types, SCSI host device XML supports
shareable tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 9 ++---
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 --
src/security/security_selinux.c | 6 --
src/util/virscsi.c | 11 ++-
src/util/virscsi.h | 4 +++-
8 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fbc9e11..65d1bde 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1674,6 +1674,7 @@ virSCSIDeviceGetDevName;
virSCSIDeviceGetName;
virSCSIDeviceGetReadonly;
virSCSIDeviceGetSgName;
+virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
virSCSIDeviceGetUsedBy;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a18955e..10b1131 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -295,7 +295,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
- dev-readonly)) == NULL)
+ dev-readonly,
+ dev-shareable)) == NULL)
goto cleanup;
if (virSCSIDeviceFileIterate(scsi,
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dee61e7..86a463a 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -267,7 +267,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit,
- hostdev-readonly)))
+ hostdev-readonly,
+ hostdev-shareable)))
goto cleanup;
virSCSIDeviceSetUsedBy(scsi, def-name);
@@ -1097,7 +1098,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit,
- hostdev-readonly)))
+ hostdev-readonly,
+ hostdev-shareable)))
goto cleanup;
if (scsi virSCSIDeviceListAdd(list, scsi) 0) {
@@ -1395,7 +1397,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit,
- hostdev-readonly))) {
+ hostdev-readonly,
+ hostdev-shareable))) {
VIR_WARN(Unable to reattach SCSI device %s:%d:%d:%d on domain %s,
hostdev-source.subsys.u.scsi.adapter,
hostdev-source.subsys.u.scsi.bus,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index a9f04d2..86a033f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -833,7 +833,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
- dev-readonly);
+ dev-readonly,
+ dev-shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cb7d322..0952df9 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -536,7 +536,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr
mgr,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
- dev-readonly);
+ dev-readonly,
+ dev-shareable);
if (!scsi)
goto done;
@@ -653,7 +654,8 @@
Re: [libvirt] [PATCH] util: Use new array management macros
On 07/01/14 23:16, Eric Blake wrote:
On 01/07/2014 08:03 AM, Osier Yang wrote:
Like commit 94a26c7e from Eric Blake, the old fuzzy code should
be replaced by the new array management macros now.
And the type of scsi-count should be changed into size_t, and
thus virSCSIDeviceListCount should return size_t instead, similar
for vir{PCI,USB}DeviceListCount.
---
src/util/virpci.c | 2 +-
src/util/virpci.h | 2 +-
src/util/virscsi.c | 35 ++-
src/util/virscsi.h | 2 +-
src/util/virusb.c | 2 +-
src/util/virusb.h | 2 +-
6 files changed, 15 insertions(+), 30 deletions(-)
Not quite.
@@ -387,24 +382,14 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
size_t i;
for (i = 0; i list-count; i++) {
-if (list-devs[i]-adapter != dev-adapter ||
-list-devs[i]-bus != dev-bus ||
-list-devs[i]-target != dev-target ||
-list-devs[i]-unit != dev-unit)
In the old code, if 3 of the 4 items match, but the fourth does not,
then we skip that item.
-continue;
-
-ret = list-devs[i];
-
-if (i != list-count--)
-memmove(list-devs[i],
-list-devs[i+1],
-sizeof(*list-devs) * (list-count - i));
-
-if (VIR_REALLOC_N(list-devs, list-count) 0) {
-; /* not fatal */
+if (list-devs[i]-adapter == dev-adapter ||
+list-devs[i]-bus == dev-bus ||
+list-devs[i]-target == dev-target ||
+list-devs[i]-unit == dev-unit) {
In the new code as written, only 1 of the 4 items has to match. You
applied deMorgan's theorem incorrectly; this must be:
I was too quick, fixed and pushed. Thanks.
Regards.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
It doesn't make sense to fail if the SCSI host device is specified
as shareable explicitly between domains (NB, it works if and only
if the device is specified as shareable for *all* domains,
otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable). And the change on the data struct brings on many
subsequent changes in the code.
* src/util/virscsi.h:
- Remove virSCSIDeviceGetUsedBy
- Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
- Add virSCSIDeviceIsAvailable
* src/util/virscsi.c:
- struct virSCSIDevice: Change used_by to be an array; Add
n_used_by as the array count
- virSCSIDeviceGetUsedBy: Removed
- virSCSIDeviceFree: frees the used_by array
- virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
memory corruption
- virSCSIDeviceIsAvailable: New
- virSCSIDeviceListDel: Change the logic, for device which is already
in the list, just remove the corresponding entry in used_by
- Copyright updating
* src/libvirt_private.sys:
- virSCSIDeviceGetUsedBy: Remove
- virSCSIDeviceIsAvailable: New
* src/qemu/qemu_hostdev.c:
- qemuUpdateActiveScsiHostdevs: Check if the device existing before
adding it to the list;
- qemuPrepareHostdevSCSIDevices: Error out if the not all domains
use the device as shareable; Also don't try to add the device
to the activeScsiHostdevs list if it already there.
- qemuDomainReAttachHostScsiDevices: Change the logic according
to the changes on helpers.
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 75 ++--
src/util/virscsi.c | 48 +--
src/util/virscsi.h | 7 +++--
4 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65d1bde..bd5f466 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1677,7 +1677,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..9d81e94 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+virSCSIDevicePtr scsi = NULL;
+virSCSIDevicePtr tmp = NULL;
if (!def-nhostdevs)
return 0;
virObjectLock(driver-activeScsiHostdevs);
for (i = 0; i def-nhostdevs; i++) {
-virSCSIDevicePtr scsi = NULL;
hostdev = def-hostdevs[i];
if (hostdev-mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev-shareable)))
goto cleanup;
-virSCSIDeviceSetUsedBy(scsi, def-name);
-
-if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0) {
+if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
+if (virSCSIDeviceSetUsedBy(tmp, def-name) 0) {
+virSCSIDeviceFree(scsi);
+goto cleanup;
+}
virSCSIDeviceFree(scsi);
-goto cleanup;
+} else {
+if (virSCSIDeviceSetUsedBy(scsi, def-name) 0 ||
+virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0) {
+virSCSIDeviceFree(scsi);
+goto cleanup;
+}
}
}
ret = 0;
@@ -1118,24 +1126,26 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
for (i = 0; i count; i++) {
virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i);
if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
-const char *other_name = virSCSIDeviceGetUsedBy(tmp);
-
-if (other_name)
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(SCSI device %s is in use by domain %s),
- virSCSIDeviceGetName(tmp), other_name);
-else
+if (!(virSCSIDeviceGetShareable(scsi)
+ virSCSIDeviceGetShareable(tmp))) {
virReportError(VIR_ERR_OPERATION_INVALID,
- _(SCSI device %s is already in use),
+ _(SCSI device %s is already in use
+ by other domain(s)),
virSCSIDeviceGetName(tmp));
-goto error;
-}
+goto error;
+}
-virSCSIDeviceSetUsedBy(scsi, name);
-VIR_DEBUG(Adding
Re: [libvirt] [PATCH 0/2 v2] Fixes on shareable SCSI host device
On 08/01/14 22:51, Osier Yang wrote: Forgot to mention that the lacked tests for scsi utils will come soon. *** BLURB HERE *** Osier Yang (2): util: Add shareable field for virSCSIDevice struct qemu: Don't fail if the SCSI host device is shareable between domains src/libvirt_private.syms | 3 +- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_hostdev.c | 84 ++-- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 ++- src/security/security_selinux.c | 6 ++- src/util/virscsi.c | 59 +++- src/util/virscsi.h | 11 -- 8 files changed, 116 insertions(+), 59 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] storage: Polling the sysfs for pool with fc_host type adapter
On 07/01/14 02:30, John Ferlan wrote: On 01/06/2014 05:19 AM, Osier Yang wrote: The SCSI device corresponding to the vHBA might not show up in sysfs yet when we trying to scan the LUNs. As a result, we will end up with an empty volume set for the pool after pool-start, even if there are LUNs. So what causes the delay to get the LUN's into sysfs? It's basicly from the delay of udev. Is there something that can be done at creation time (or wherever) to sync that sooner? I thought like that, let's say at the point of createVport. But the createVport is just to create the vHBA, and nothing else to do, the left work for device showing up in the sysfs tree is on the udev then. Polling right after createVport for the SCSI device in sysfs tree will take more time. Is there a way to determine that the SCSI device hasn't shown up yet other than the readdir()? You're adding a delay/loop for some other subsystem's inability? to sync or provide the resources. It's the only way AFAIK. Though the time of the device showing up is rather depended, better than doing nothing, this patch introduces the polling with 5 * 1 seconds in maximum (the time works fine on my testing machine at least). Note that for the pool which doesn't have any LUN, it will still take 5 seconds to poll, but it's not a bad trade, 5 seconds is not much, and in most cases, one won't use an empty pool in practice. Since the paths that call into this only seem to be via refresh and perhaps a subsequent refresh would resolve things. Exactly, in most cases it will work, since the time window between pool-start and pool-refresh should be enough for the SCSI device showing up in the sysfs tree, *generally*. but it's not necessary to be that. Could this be better served by documenting that it's possible that depending on circumstance X (answer to my first question) that in order to see elements in the pool, one may have to reload again. Assuming of course that the next reload would find them... I thought like this too, but the problem is it's not guaranteed that the volume could be loaded after execute pool-refresh one time, may be 2 times, 3 times, ... N times. Also the time of each pool-refresh is not fixed, it depends on how long the udevadm settle (see virFileWaitForDevices) will take. I guess I'm a bit cautious about adding randomly picked timeout values based on some test because while it may work for you, perhaps it's 10 seconds for someone else. While you may consider a 5 second pause OK and reasonable a customer may not consider that to be reasonable. People (and testers) do strange and random things. Exactly, this is not the only problem we faced regarding to the storage stuffs, and the users keeps asking why, why, and why. Furthermore, could it be possible that you catch things perfectly and only say 10 of 100 devices are found... But if you waited another 5 seconds the other 90 devices would show up.. I think by adding this code you end up down a slippery slope of handing fc_host devices specially... We are exactly on the same page, but the question is what the best solution we should provide? It looks ugly if we add documentation saying one should use pool-refresh after the pool is started, if the volumes are not loaded, but how many times to use the pool-refresh is depended? This patch was basicly a proposal for discussion. I didn't expect it could be committed smoothly. Regards, Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] storage: Fix autostart of pool with fc_host type adapter
On 07/01/14 01:21, John Ferlan wrote: On 01/06/2014 05:19 AM, Osier Yang wrote: The checkPool is a bit different for pool with fc_host type source adapter, since the vHBA it's based on might be not created yet (it's created by startPool, which is involked after checkPool in storageDriverAutostart). So it should not fail, otherwise the autostart of the pool will fail either. The problem is easy to reproduce: * Enable autostart for the pool * Restart libvirtd service * Check the pool's state --- src/storage/storage_backend_scsi.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) Not sure this is the right thing to do. With this change it doesn't matter what getAdapterName() returns for fc_host's... It matters with if *isActive is true or false in the end. We don't need to try to start the pool if it's already active. The getAdapterName() already has some code to specifically handle VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST. The call to virGetFCHostNameByWWN() is similar to createVport() which is called by the 'start' path, except that the createVport() path will be happy if the name already exists. Since it seems the checkPool is meant to initialize things (from reading the error message in the calling function), why not create the vPort in the check function? It's a bit more refactoring that perhaps initially desired, although having a vport hanging around unused may not be quite right either. No, checkPool is only involked when autostarting the pools. Not by poolCreate, poolCreateXML, etc. That's why creating pool must fall into startPool. Another option would be to have the check function perform enough initialization or checks to make it more likely that the start path will succeed. That's actually what checkPool *should* do. But for fc_host pool, it's a bit special, since it's unlike other pool types, the underlying physical stuffs must be already existing on host. Looking at the code does cause me to wonder what happens in the existing code if the vport was created when the CheckPool function was called returning the NameByWWN() in the 'name' field. The subsequent getHostNumber() call uses the 'name' instead of what the start path does using the parent value. It's right. getHostNumber in checkPool is to get the SCSI host number of the vHBA. And then checking if the SCSI device shows up in the sysfs tree with the got host number. getHostNumber in startPool should get the parent's host number, since it should know the sys file path /.../.../create_vport. And write command to it. It seems the check for fc_host and non-fc_host is different enough that the distinguishment needs to be in the check routine rather than hidden in the getAdapterName() function. Not quite clear about your meaning. But getAdapterName is just a wrapper to get the adapter name with regarding to different adapter type. Any relationship of the check difference ? Regards, Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 06/01/14 23:54, John Ferlan wrote:
On 01/02/2014 09:45 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is specified
as shareable explicitly between domains (NB, it works if and only
if the device is specified as shareable for *all* domains,
otherwise it fails).
Also don't try to add the device to the activeScsiHostdevs list if
it's already there.
---
src/qemu/qemu_hostdev.c | 33 ++---
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..8536499 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -1120,22 +1120,25 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
const char *other_name = virSCSIDeviceGetUsedBy(tmp);
-if (other_name)
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(SCSI device %s is in use by domain %s),
- virSCSIDeviceGetName(tmp), other_name);
-else
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(SCSI device %s is already in use),
- virSCSIDeviceGetName(tmp));
-goto error;
-}
-
-virSCSIDeviceSetUsedBy(scsi, name);
-VIR_DEBUG(Adding %s to activeScsiHostdevs,
virSCSIDeviceGetName(scsi));
+if (!(virSCSIDeviceGetShareable(scsi)
+ virSCSIDeviceGetShareable(tmp))) {
+if (other_name)
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _(SCSI device %s is in use by domain %s),
+ virSCSIDeviceGetName(tmp), other_name);
+else
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _(SCSI device %s is already in use),
+ virSCSIDeviceGetName(tmp));
+goto error;
+}
+} else {
+virSCSIDeviceSetUsedBy(scsi, name);
+VIR_DEBUG(Adding %s to activeScsiHostdevs,
virSCSIDeviceGetName(scsi));
-if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0)
-goto error;
+if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0)
+goto error;
+}
Something doesn't feel right here...
Prior code:
if find device on some other drivers active list
fail
set name of who device is used by
add device to host active list
(NOTE: will error if found on list already)
New code
if find device on some other drivers active list
if both devices not marked shareable
fail
else
set name of who device is used by
add device to host active list
So now theoretically with a shareable device, the device could be on the
active list and it's OK to use it because it's marked shareable.
Yes, as long as the later domain(s) specify the device as shareable
too.
So my question/issue becomes what to do with the dev-used_by (eg,
virSCSIDeviceSetUsedBy() result) value. In fact, since we're now
shareable it seems to me that we'd need to keep this up to date even
going so far as to clear usage it when virSCSIDeviceListDel() is called.
Should the list become a list of those using it?
/Alarm
Yes, thanks for pointing it out...
The first domain
that claims usage could eventually remove their usage, then what? It
would be strange to report domain X has the device in use if the domain
is down or doesn't exist.
It actually won't report error. Since the dev-used_by is freed
along with the domain destroying. E.g.
% start domain A with sg7 as shareable.
% start domain B with sg7 as shareable
% Destroy domain A
% start domain C with sg7 as non-shareable.
Starting Domain C is expected to fail, but it will succeed.
I will post the v2.
Use cscope and check the callers/users of
used_by to see what my concern is.
Remember prior to this point in time we cannot share, so it didn't
matter. However, from this point on since we can share we need to keep
track of who has it, right? What becomes even more interesting perhaps
is the impact on migration (if that's even possible with a shared hostdev).
It should be fine for migration, as long as we make the used_by
correct.
Regards,
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Use new array management macros
Like commit 94a26c7e from Eric Blake, the old fuzzy code should
be replaced by the new array management macros now.
And the type of scsi-count should be changed into size_t, and
thus virSCSIDeviceListCount should return size_t instead, similar
for vir{PCI,USB}DeviceListCount.
---
src/util/virpci.c | 2 +-
src/util/virpci.h | 2 +-
src/util/virscsi.c | 35 ++-
src/util/virscsi.h | 2 +-
src/util/virusb.c | 2 +-
src/util/virusb.h | 2 +-
6 files changed, 15 insertions(+), 30 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 8ec642f..ea93771 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1721,7 +1721,7 @@ virPCIDeviceListGet(virPCIDeviceListPtr list,
return list-devs[idx];
}
-int
+size_t
virPCIDeviceListCount(virPCIDeviceListPtr list)
{
return list-count;
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 0479f0b..08bf4c3 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -86,7 +86,7 @@ int virPCIDeviceListAdd(virPCIDeviceListPtr list,
int virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev);
virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr list,
int idx);
-int virPCIDeviceListCount(virPCIDeviceListPtr list);
+size_t virPCIDeviceListCount(virPCIDeviceListPtr list);
virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list,
virPCIDevicePtr dev);
virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list,
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 7aca9e6..f12a2a5 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -62,7 +62,7 @@ struct _virSCSIDevice {
struct _virSCSIDeviceList {
virObjectLockable parent;
-unsigned int count;
+size_t count;
virSCSIDevicePtr *devs;
};
@@ -356,12 +356,7 @@ virSCSIDeviceListAdd(virSCSIDeviceListPtr list,
return -1;
}
-if (VIR_REALLOC_N(list-devs, list-count + 1) 0)
-return -1;
-
-list-devs[list-count++] = dev;
-
-return 0;
+return VIR_APPEND_ELEMENT(list-devs, list-count, dev);
}
virSCSIDevicePtr
@@ -373,7 +368,7 @@ virSCSIDeviceListGet(virSCSIDeviceListPtr list, int idx)
return list-devs[idx];
}
-int
+size_t
virSCSIDeviceListCount(virSCSIDeviceListPtr list)
{
return list-count;
@@ -387,24 +382,14 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
size_t i;
for (i = 0; i list-count; i++) {
-if (list-devs[i]-adapter != dev-adapter ||
-list-devs[i]-bus != dev-bus ||
-list-devs[i]-target != dev-target ||
-list-devs[i]-unit != dev-unit)
-continue;
-
-ret = list-devs[i];
-
-if (i != list-count--)
-memmove(list-devs[i],
-list-devs[i+1],
-sizeof(*list-devs) * (list-count - i));
-
-if (VIR_REALLOC_N(list-devs, list-count) 0) {
-; /* not fatal */
+if (list-devs[i]-adapter == dev-adapter ||
+list-devs[i]-bus == dev-bus ||
+list-devs[i]-target == dev-target ||
+list-devs[i]-unit == dev-unit) {
+ret = list-devs[i];
+VIR_DELETE_ELEMENT(list-devs, i, list-count);
+break;
}
-
-break;
}
return ret;
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index cce5df4..4c461f8 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -77,7 +77,7 @@ int virSCSIDeviceListAdd(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
virSCSIDevicePtr virSCSIDeviceListGet(virSCSIDeviceListPtr list,
int idx);
-int virSCSIDeviceListCount(virSCSIDeviceListPtr list);
+size_t virSCSIDeviceListCount(virSCSIDeviceListPtr list);
virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
void virSCSIDeviceListDel(virSCSIDeviceListPtr list,
diff --git a/src/util/virusb.c b/src/util/virusb.c
index 3c82200..bb5980d 100644
--- a/src/util/virusb.c
+++ b/src/util/virusb.c
@@ -464,7 +464,7 @@ virUSBDeviceListGet(virUSBDeviceListPtr list,
return list-devs[idx];
}
-int
+size_t
virUSBDeviceListCount(virUSBDeviceListPtr list)
{
return list-count;
diff --git a/src/util/virusb.h b/src/util/virusb.h
index aa59d12..e0a7c4c 100644
--- a/src/util/virusb.h
+++ b/src/util/virusb.h
@@ -86,7 +86,7 @@ int virUSBDeviceListAdd(virUSBDeviceListPtr list,
virUSBDevicePtr dev);
virUSBDevicePtr virUSBDeviceListGet(virUSBDeviceListPtr list,
int idx);
-int virUSBDeviceListCount(virUSBDeviceListPtr list);
+size_t virUSBDeviceListCount(virUSBDeviceListPtr list);
virUSBDevicePtr virUSBDeviceListSteal(virUSBDeviceListPtr list,
virUSBDevicePtr dev);
void virUSBDeviceListDel(virUSBDeviceListPtr list,
--
[libvirt] [PATCH 0/2] Storage: Fixes for the fc_host type pool
*** Enough details in the patch commits *** Osier Yang (2): storage: Fix autostart of pool with fc_host type adapter storage: Polling the sysfs for pool with fc_host type adapter src/storage/storage_backend_scsi.c | 30 -- 1 file changed, 28 insertions(+), 2 deletions(-) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] storage: Polling the sysfs for pool with fc_host type adapter
The SCSI device corresponding to the vHBA might not show up in
sysfs yet when we trying to scan the LUNs. As a result, we will
end up with an empty volume set for the pool after pool-start,
even if there are LUNs.
Though the time of the device showing up is rather depended,
better than doing nothing, this patch introduces the polling
with 5 * 1 seconds in maximum (the time works fine on my
testing machine at least). Note that for the pool which doesn't
have any LUN, it will still take 5 seconds to poll, but it's
not a bad trade, 5 seconds is not much, and in most cases,
one won't use an empty pool in practice.
---
src/storage/storage_backend_scsi.c | 16
1 file changed, 16 insertions(+)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 93039c1..2efcdb8 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -495,6 +495,8 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
DIR *devicedir = NULL;
struct dirent *lun_dirent = NULL;
char devicepattern[64];
+bool found = false;
+size_t i = 0;
VIR_DEBUG(Discovering LUs on host %u, scanhost);
@@ -510,6 +512,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
snprintf(devicepattern, sizeof(devicepattern), %u:%%u:%%u:%%u\n,
scanhost);
+retry:
while ((lun_dirent = readdir(devicedir))) {
if (sscanf(lun_dirent-d_name, devicepattern,
bus, target, lun) != 3) {
@@ -518,9 +521,22 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool,
VIR_DEBUG(Found LU '%s', lun_dirent-d_name);
+found = true;
processLU(pool, scanhost, bus, target, lun);
}
+/* Sleep for 5 seconds in maximum if the pool's source
+ * adapter type is fc_host, since the corresponding
+ * SCSI device might not show up in the sysfs yet.
+ */
+if (!found i++ 5
+pool-def-source.adapter.type ==
+VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
+sleep(1);
+rewinddir(devicedir);
+goto retry;
+}
+
closedir(devicedir);
return retval;
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] storage: Fix autostart of pool with fc_host type adapter
The checkPool is a bit different for pool with fc_host
type source adapter, since the vHBA it's based on might be
not created yet (it's created by startPool, which is
involked after checkPool in storageDriverAutostart). So it
should not fail, otherwise the autostart of the pool will
fail either.
The problem is easy to reproduce:
* Enable autostart for the pool
* Restart libvirtd service
* Check the pool's state
---
src/storage/storage_backend_scsi.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 6f86ffc..93039c1 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -702,8 +702,18 @@ virStorageBackendSCSICheckPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
*isActive = false;
-if (!(name = getAdapterName(pool-def-source.adapter)))
-return -1;
+if (!(name = getAdapterName(pool-def-source.adapter))) {
+/* It's normal for the pool with fc_host type source
+ * adapter fails to get the adapter name, since the vHBA
+ * the adapter based on might be not created yet.
+ */
+if (pool-def-source.adapter.type ==
+VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
+return 0;
+} else {
+return -1;
+}
+}
if (getHostNumber(name, host) 0)
goto cleanup;
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] event: use bool in more places
On 04/01/14 03:31, Eric Blake wrote: No need to use an int that only ever stores 0 and 1. * src/conf/object_event_private.h (_virObjectEventCallback): Change deleted to bool. * src/conf/object_event.c (virObjectEventDispatchMatchCallback): Switch return type to bool. (virObjectEventCallbackListMarkDeleteID): Update client. * src/conf/domain_event.c (virDomainEventCallbackListMarkDelete): Likewise. --- src/conf/domain_event.c | 2 +- src/conf/object_event.c | 20 src/conf/object_event_private.h | 2 +- 3 files changed, 10 insertions(+), 14 deletions(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] event: make deregister return value match docs
On 04/01/14 03:31, Eric Blake wrote: Ever since their introduction (commit 1509b80 in v0.5.0 for virConnectDomainEventRegister, commit 4445723 in v0.8.0 for virConnectDomainEventDeregisterAny), the event deregistration functions have been documented as returning 0 on success; likewise for older registration (only the newer RegisterAny must return a non-zero callbackID). And now that we are adding virConnectNetworkEventDeregisterAny for v1.2.1, it should have the same semantics. Fortunately, all of the stateful drivers have been obeying the docs and returning 0, thanks to the way the remote_driver tracks things (in fact, the RPC wire protocol is unable to send a return value for DomainEventRegisterAny, at least not without adding a new RPC number). But for local drivers, such as test:///default, we've been returning non-zero numbers; worse, the non-zero numbers have differed over time. For example, in Fedora 12 (libvirt 0.8.2), calling Register twice would return 0 and 1 [the callbackID generated under the hood]; while in Fedora 20 (libvirt 1.1.3), it returns 1 and 2 [the number of callbacks registered for that event type]. Since we have changed the behavior over time, and since it differs by local vs. remote, we can safely argue that no one could have been reasonably relying on any particular behavior, so we might as well obey the docs, as well as prepare callers that might deal with older clients to not be surprised if the docs are not strictly followed. For consistency, this patch fixes the code for all drivers, even though it only makes an impact for local drivers; that way, future copy and paste from a remote driver to a local driver is less likely to reintroduce the bug. * src/libvirt.c (virConnectDomainEventRegister) (virConnectDomainEventDeregister) (virConnectDomainEventDeregisterAny): Clarify docs. * src/libxl/libxl_driver.c (libxlConnectDomainEventRegister) (libxlConnectDomainEventDeregister) (libxlConnectDomainEventDeregisterAny): Match documentation. * src/lxc/lxc_driver.c (lxcConnectDomainEventRegister) (lxcConnectDomainEventDeregister) (lxcConnectDomainEventDeregisterAny): Likewise. * src/test/test_driver.c (testConnectDomainEventRegister) (testConnectDomainEventDeregister) (testConnectDomainEventDeregisterAny) (testConnectNetworkEventDeregisterAny): Likewise. * src/uml/uml_driver.c (umlConnectDomainEventRegister) (umlConnectDomainEventDeregister) (umlConnectDomainEventDeregisterAny): Likewise. * src/vbox/vbox_tmpl.c (vboxConnectDomainEventRegister) (vboxConnectDomainEventDeregister) (vboxConnectDomainEventDeregisterAny): Likewise. * src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister) (xenUnifiedConnectDomainEventDeregister) (xenUnifiedConnectDomainEventDeregisterAny): Likewise. * src/network/bridge_driver.c (networkConnectNetworkEventDeregisterAny): Likewise. Signed-off-by: Eric Blake ebl...@redhat.com --- src/libvirt.c | 17 +++-- src/libxl/libxl_driver.c| 35 ++- src/lxc/lxc_driver.c| 32 src/network/bridge_driver.c | 11 +++ src/test/test_driver.c | 38 +- src/uml/uml_driver.c| 29 - src/vbox/vbox_tmpl.c| 32 ++-- src/xen/xen_driver.c| 27 +++ 8 files changed, 126 insertions(+), 95 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index a0a26e5..f527dcc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16363,7 +16363,9 @@ error: * The reference can be released once the object is no longer required * by calling virDomainFree. * - * Returns 0 on success, -1 on failure. + * Returns 0 on success, -1 on failure. Older versions of some hypervisors + * sometimes returned a positive number on success, but without any reliable + * semantics on what that number represents. */ int virConnectDomainEventRegister(virConnectPtr conn, @@ -16401,14 +16403,16 @@ error: * @conn: pointer to the connection * @cb: callback to the function handling domain events * - * Removes a callback previously registered with the virConnectDomainEventRegister - * function. + * Removes a callback previously registered with the + * virConnectDomainEventRegister() function. * * Use of this method is no longer recommended. Instead applications * should try virConnectDomainEventDeregisterAny() which has a more flexible * API contract * - * Returns 0 on success, -1 on failure + * Returns 0 on success, -1 on failure. Older versions of some hypervisors + * sometimes returned a positive number on success, but without any reliable + * semantics on what that number represents. */ int virConnectDomainEventDeregister(virConnectPtr conn, @@ -19443,8 +19447,9 @@ error: * Removes an event callback. The callbackID parameter should be the * value obtained from a previous
[libvirt] [PATCH 0/2] Fixes on shareable SCSI host device
It's easy to reproduce with two domains using the same SCSI generic device, both with shareable specified. It's expected to work. But the fact is: % virsh start f20-1 error: Failed to start domain b error: Requested operation is not valid: SCSI device 1:0:0:0 is in use by domain f20-0 Osier Yang (2): util: Add shareable field for virSCSIDevice struct qemu: Don't fail if the SCSI host device is shareable between domains src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 3 ++- src/qemu/qemu_hostdev.c | 42 +++- src/security/security_apparmor.c | 3 ++- src/security/security_dac.c | 6 -- src/security/security_selinux.c | 6 -- src/util/virscsi.c | 11 ++- src/util/virscsi.h | 4 +++- 8 files changed, 50 insertions(+), 26 deletions(-) -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
It doesn't make sense to fail if the SCSI host device is specified
as shareable explicitly between domains (NB, it works if and only
if the device is specified as shareable for *all* domains,
otherwise it fails).
Also don't try to add the device to the activeScsiHostdevs list if
it's already there.
---
src/qemu/qemu_hostdev.c | 33 ++---
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..8536499 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -1120,22 +1120,25 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
if ((tmp = virSCSIDeviceListFind(driver-activeScsiHostdevs, scsi))) {
const char *other_name = virSCSIDeviceGetUsedBy(tmp);
-if (other_name)
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(SCSI device %s is in use by domain %s),
- virSCSIDeviceGetName(tmp), other_name);
-else
-virReportError(VIR_ERR_OPERATION_INVALID,
- _(SCSI device %s is already in use),
- virSCSIDeviceGetName(tmp));
-goto error;
-}
-
-virSCSIDeviceSetUsedBy(scsi, name);
-VIR_DEBUG(Adding %s to activeScsiHostdevs,
virSCSIDeviceGetName(scsi));
+if (!(virSCSIDeviceGetShareable(scsi)
+ virSCSIDeviceGetShareable(tmp))) {
+if (other_name)
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _(SCSI device %s is in use by domain %s),
+ virSCSIDeviceGetName(tmp), other_name);
+else
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _(SCSI device %s is already in use),
+ virSCSIDeviceGetName(tmp));
+goto error;
+}
+} else {
+virSCSIDeviceSetUsedBy(scsi, name);
+VIR_DEBUG(Adding %s to activeScsiHostdevs,
virSCSIDeviceGetName(scsi));
-if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0)
-goto error;
+if (virSCSIDeviceListAdd(driver-activeScsiHostdevs, scsi) 0)
+goto error;
+}
}
virObjectUnlock(driver-activeScsiHostdevs);
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: Add shareable field for virSCSIDevice struct
Unlike the host devices of other types, SCSI host device XML supports
shareable tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 9 ++---
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 --
src/security/security_selinux.c | 6 --
src/util/virscsi.c | 11 ++-
src/util/virscsi.h | 4 +++-
8 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2dbb8f8..68ca5da 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1671,6 +1671,7 @@ virSCSIDeviceGetDevName;
virSCSIDeviceGetName;
virSCSIDeviceGetReadonly;
virSCSIDeviceGetSgName;
+virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
virSCSIDeviceGetUsedBy;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a18955e..10b1131 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -295,7 +295,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
- dev-readonly)) == NULL)
+ dev-readonly,
+ dev-shareable)) == NULL)
goto cleanup;
if (virSCSIDeviceFileIterate(scsi,
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dee61e7..86a463a 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -267,7 +267,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit,
- hostdev-readonly)))
+ hostdev-readonly,
+ hostdev-shareable)))
goto cleanup;
virSCSIDeviceSetUsedBy(scsi, def-name);
@@ -1097,7 +1098,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit,
- hostdev-readonly)))
+ hostdev-readonly,
+ hostdev-shareable)))
goto cleanup;
if (scsi virSCSIDeviceListAdd(list, scsi) 0) {
@@ -1395,7 +1397,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
hostdev-source.subsys.u.scsi.bus,
hostdev-source.subsys.u.scsi.target,
hostdev-source.subsys.u.scsi.unit,
- hostdev-readonly))) {
+ hostdev-readonly,
+ hostdev-shareable))) {
VIR_WARN(Unable to reattach SCSI device %s:%d:%d:%d on domain %s,
hostdev-source.subsys.u.scsi.adapter,
hostdev-source.subsys.u.scsi.bus,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index a9f04d2..86a033f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -833,7 +833,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
- dev-readonly);
+ dev-readonly,
+ dev-shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cb7d322..0952df9 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -536,7 +536,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr
mgr,
dev-source.subsys.u.scsi.bus,
dev-source.subsys.u.scsi.target,
dev-source.subsys.u.scsi.unit,
- dev-readonly);
+ dev-readonly,
+ dev-shareable);
if (!scsi)
goto done;
@@ -653,7 +654,8 @@
Re: [libvirt] [PATCH] Doc: Explicitly declaring that nodedev-destroy only works for vHBA
On 03/12/13 00:56, Eric Blake wrote: On 11/22/2013 05:55 AM, Osier Yang wrote: Though trying to destroy a physical HBA doesn't make sense at all, it's still a bit misleading with saying only works for HBA. Signed-off-by: Osier Yang jy...@redhat.com --- src/libvirt.c | 5 +++-- tools/virsh.pod | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) ACK Pushed, Thanks. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Doc: Explicitly declaring that nodedev-destroy only works for vHBA
On 22/11/13 20:55, Osier Yang wrote: Can someone give an obvious ACK? Though trying to destroy a physical HBA doesn't make sense at all, it's still a bit misleading with saying only works for HBA. Signed-off-by: Osier Yang jy...@redhat.com --- src/libvirt.c | 5 +++-- tools/virsh.pod | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ae05300..4ebd13f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16065,8 +16065,9 @@ error: * virNodeDeviceDestroy: * @dev: a device object * - * Destroy the device object. The virtual device is removed from the host operating system. - * This function may require privileged access + * Destroy the device object. The virtual device (only works for vHBA + * currently) is removed from the host operating system. This function + * may require privileged access. * * Returns 0 in case of success and -1 in case of failure. */ diff --git a/tools/virsh.pod b/tools/virsh.pod index dac9a08..557df9c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2158,9 +2158,9 @@ contains xml for a top-level device description of a node device. =item Bnodedev-destroy Idevice Destroy (stop) a device on the host. Idevice can be either device -name or wwn pair in wwnn,wwpn format (only works for HBA). Note -that this makes libvirt quit managing a host device, and may even make -that device unusable by the rest of the physical host until a reboot. +name or wwn pair in wwnn,wwpn format (only works for vHBA currently). +Note that this makes libvirt quit managing a host device, and may even +make that device unusable by the rest of the physical host until a reboot. =item Bnodedev-detach Inodedev [I--driver backend_driver] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
On 27/11/13 17:58, Peter Krempa wrote:
On 11/27/13 08:47, Osier Yang wrote:
On 27/11/13 00:48, Peter Krempa wrote:
To support testing of volume disk backing, we need to implement a few
disk driver backend functions.
The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
as XML files for pool definitions and volume names are in format
VOL_TYPE+VOL_PATH. By default type block is assumed (for iSCSI test
compatibility).
The choice of this approach along with implemented functions was made so
that disk type='volume' can be tested in the xml2argv test.
---
tests/qemuxml2argvtest.c | 162
+++
1 file changed, 162 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a290062..a4cef84 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -18,6 +18,7 @@
# include qemu/qemu_command.h
# include qemu/qemu_domain.h
# include datatypes.h
+# include conf/storage_conf.h
# include cpu/cpu_map.h
# include virstring.h
@@ -75,6 +76,161 @@ static virSecretDriver fakeSecretDriver = {
.secretUndefine = NULL,
};
+
+# define STORAGE_POOL_XML_PATH storagepoolxml2xmlout/
This will cause build failure when building with VPATH.
Hmmm, I'll look into it.
FYI, You can use abs_srcdir directly to construct the path now, after
Eric's patch is pushed:
https://www.redhat.com/archives/libvir-list/2013-November/msg01265.html
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 1/4] test: Implement fake storage pool driver in qemuxml2argv test
On 27/11/13 23:14, Peter Krempa wrote:
To support testing of volume disk backing, we need to implement a few
disk driver backend functions.
The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
as XML files for pool definitions and volume names are in format
VOL_TYPE+VOL_PATH. By default type block is assumed (for iSCSI test
compatibility).
The choice of this approach along with implemented functions was made so
that disk type='volume' can be tested in the xml2argv test.
---
tests/qemuxml2argvtest.c | 183 +++
1 file changed, 183 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a290062..9c8f8e2 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -18,6 +18,7 @@
# include qemu/qemu_command.h
# include qemu/qemu_domain.h
# include datatypes.h
+# include conf/storage_conf.h
# include cpu/cpu_map.h
# include virstring.h
@@ -75,6 +76,182 @@ static virSecretDriver fakeSecretDriver = {
.secretUndefine = NULL,
};
+
+# define STORAGE_POOL_XML_PATH storagepoolxml2xmlout/
+static const unsigned char fakeUUID[VIR_UUID_BUFLEN] = fakeuuid;
+
+static virStoragePoolPtr
+fakeStoragePoolLookupByName(virConnectPtr conn,
+const char *name)
+{
+char *xmlpath = NULL;
+virStoragePoolPtr ret = NULL;
+
+if (STRNEQ(name, inactive)) {
+if (virAsprintf(xmlpath, %s/%s%s.xml,
+abs_srcdir,
+STORAGE_POOL_XML_PATH,
+name) 0)
Ok, this solves the build problem.
+return NULL;
+
+if (!virFileExists(xmlpath)) {
+virReportError(VIR_ERR_NO_STORAGE_POOL,
+ File '%s' not found, xmlpath);
+goto cleanup;
+}
+}
+
+ret = virGetStoragePool(conn, name, fakeUUID, NULL, NULL);
+
+cleanup:
+VIR_FREE(xmlpath);
+return ret;
+}
+
+
+static virStorageVolPtr
+fakeStorageVolLookupByName(virStoragePoolPtr pool,
+ const char *name)
+{
+char **volinfo = NULL;
+virStorageVolPtr ret = NULL;
+
+if (STREQ(pool-name, inactive)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ storage pool '%s' is not active, pool-name);
+return NULL;
+}
+
+if (STREQ(name, nonexistent)) {
+virReportError(VIR_ERR_NO_STORAGE_VOL,
+ no storage vol with matching name '%s', name);
+return NULL;
+}
+
+if (!strchr(name, '+'))
+goto fallback;
+
+if (!(volinfo = virStringSplit(name, +, 2)))
+return NULL;
+
+if (!volinfo[1])
+goto fallback;
+
+ret = virGetStorageVol(pool-conn, pool-name, volinfo[1], volinfo[0],
+ NULL, NULL);
+
+cleanup:
+virStringFreeList(volinfo);
+return ret;
+
+fallback:
+ret = virGetStorageVol(pool-conn, pool-name, name, block, NULL, NULL);
+goto cleanup;
+}
+
+static int
+fakeStorageVolGetInfo(virStorageVolPtr vol,
+ virStorageVolInfoPtr info)
+{
+memset(info, 0, sizeof(*info));
+
+info-type = virStorageVolTypeFromString(vol-key);
+
+if (info-type 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ Invalid volume type '%s', vol-key);
+return -1;
+}
+
+return 0;
+}
+
+
+static char *
+fakeStorageVolGetPath(virStorageVolPtr vol)
+{
+char *ret = NULL;
+
+ignore_value(virAsprintf(ret, /some/%s/device/%s, vol-key, vol-name));
+
+return ret;
+}
+
+
+static char *
+fakeStoragePoolGetXMLDesc(virStoragePoolPtr pool,
+ unsigned int flags_unused ATTRIBUTE_UNUSED)
+{
+char *xmlpath = NULL;
+char *xmlbuf = NULL;
+
+if (STREQ(pool-name, inactive)) {
+virReportError(VIR_ERR_NO_STORAGE_POOL, NULL);
+return NULL;
+}
+
+if (virAsprintf(xmlpath, %s/%s%s.xml,
+abs_srcdir,
+STORAGE_POOL_XML_PATH,
+pool-name) 0)
+return NULL;
+
+if (virtTestLoadFile(xmlpath, xmlbuf) 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ failed to load XML file '%s',
+ xmlpath);
+goto cleanup;
+}
+
+cleanup:
+VIR_FREE(xmlpath);
+
+return xmlbuf;
+}
+
+static int
+fakeStorageClose(virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+return 0;
+}
+
+static int
+fakeStoragePoolIsActive(virStoragePoolPtr pool)
+{
+if (STREQ(pool-name, inactive))
+return 0;
+
+return 1;
+}
+
+/* Test storage pool implementation
+ *
+ * These functions aid testing of storage pool related stuff when creating a
+ * qemu command .
s/command \./command line\./,
+ *
+ * There are a few magic values to pass to these functions:
+ *
+ * 1) inactive as
+ * a pool name for pool lookup creates a inactive pool. All other names are
s/a/an/,
How about:
a pool name to create an inactive pool.
Re: [libvirt] [PATCHv2 2/4] qemuxml2argv: Add test to verify correct usage of disk type=volume
On 27/11/13 23:14, Peter Krempa wrote: Tweak the existing file to test command line generator too. --- tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args | 8 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +- tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.args Michal's ACK stands. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 3/4] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
On 27/11/13 23:14, Peter Krempa wrote: Tweak the existing file so that it can be tested for command line corectness. --- tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args | 10 ++ tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml | 4 ++-- tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args new file mode 100644 index 000..8f6a3dd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args @@ -0,0 +1,10 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \ +file=/some/block/device/unit:0:0:1,if=none,media=cdrom,id=drive-ide0-0-1 -device \ +ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 -drive \ +file=iscsi://iscsi.example.com:3260/demo-target/2,if=none,media=cdrom,id=drive-ide0-0-2 \ +-device ide-drive,bus=ide.0,unit=2,drive=drive-ide0-0-2,id=ide0-0-2 -drive \ +file=/tmp/idedisk.img,if=none,id=drive-ide0-0-3 -device \ +ide-drive,bus=ide.0,unit=3,drive=drive-ide0-0-3,id=ide0-0-3 -device \ +virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml index b907633..9f90293 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml @@ -15,7 +15,7 @@ devices emulator/usr/bin/qemu/emulator disk type='volume' device='cdrom' - source pool='blk-pool0' volume='blk-pool0-vol0' mode='host' startupPolicy='optional' + source pool='pool-iscsi-auth' volume='unit:0:0:1' mode='host' seclabel model='selinux' relabel='yes' labelsystem_u:system_r:public_content_t:s0/label /seclabel @@ -25,7 +25,7 @@ address type='drive' controller='0' bus='0' target='0' unit='1'/ /disk disk type='volume' device='cdrom' - source pool='blk-pool0' volume='blk-pool0-vol1' mode='direct' startupPolicy='optional' + source pool='pool-iscsi' volume='unit:0:0:2' mode='direct' Okay, I see why you removed the startupPolicy now, it doesn't make sense for a pool with block type volume. ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 4/4] qemu: Refactor qemuTranslatePool source
On 27/11/13 23:14, Peter Krempa wrote:
Before this patch, the translation function still needs a second ugly
helper function to actually format the command line for qemu. But if we
do the right stuff in the translation functio, we don't have to bother
s/functio/function/,
with the second function any more.
This patch removes the messy qemuBuildVolumeString() function and
changes qemuTranslatePool() to set stuff up correctly so that the
s/qemuTranslatePool/qemuTranslateDiskSourcePool/,
regular code paths meant for volumes can be used to format the command
line correctly.
For this purpose a new helper qemuDiskGetActualType() is introduced to
return the type of the volume in a pool.
As a part of the refactor the qemuTranslatePool function is fixed to do
Same as above.
decisions based on the pool type instead of the volume type. This allows
to separate pool-type-specific stuff more clearly and will ease addition
of other pool types that will require certain other operations to get
the correct pool source.
The previously fixed tests should make sure that we don't break stuff
that was working before.
---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 76 +++-
src/qemu/qemu_conf.c | 129 ---
src/qemu/qemu_conf.h | 2 +
5 files changed, 98 insertions(+), 111 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4561ccc..a5ef2ca 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
char *volume; /* volume name */
int voltype; /* enum virStorageVolType, internal only */
int pooltype; /* enum virStoragePoolType, internal only */
+int actualtype; /* enum virDomainDiskType, internal only */
int mode; /* enum virDomainDiskSourcePoolMode */
};
typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 205fe56..aeb3568 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -693,6 +693,7 @@ virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
+virStoragePoolTypeToString;
virStorageVolDefFindByKey;
virStorageVolDefFindByName;
virStorageVolDefFindByPath;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..b8129a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn,
virDomainDiskDefPtr disk, virBufferPtr op
return 0;
}
-static int
-qemuBuildVolumeString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
I admit I was confused when reviewing v2 without the commit log.
-{
-int ret = -1;
-
-switch ((virStorageVolType) disk-srcpool-voltype) {
-case VIR_STORAGE_VOL_DIR:
-if (!disk-readonly) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
- _(cannot create virtual FAT disks in read-write
mode));
-goto cleanup;
-}
-if (disk-device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
-virBufferEscape(opt, ',', ,, file=fat:floppy:%s,, disk-src);
-else
-virBufferEscape(opt, ',', ,, file=fat:%s,, disk-src);
-break;
-case VIR_STORAGE_VOL_BLOCK:
-if (disk-tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
- _(tray status 'open' is invalid for
- block type volume));
[1]
I keep my opinion that it should provide an exact error, either block
or volume is the term we used in the XML and documents. One
will be confused to see tray status 'open' is invalid for block type disk,
since it's actually defined as type=volume. It doesn't have to be
the same, but there should be a way to differentiate them.
-goto cleanup;
-}
-if (disk-srcpool-pooltype == VIR_STORAGE_POOL_ISCSI) {
-if (disk-srcpool-mode ==
-VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
-if (qemuBuildISCSIString(conn, disk, opt) 0)
-goto cleanup;
-virBufferAddChar(opt, ',');
-} else if (disk-srcpool-mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
-virBufferEscape(opt, ',', ,, file=%s,, disk-src);
-}
-} else {
-virBufferEscape(opt, ',', ,, file=%s,, disk-src);
-}
-break;
-case VIR_STORAGE_VOL_FILE:
-if (disk-auth.username) {
-if (qemuBuildISCSIString(conn, disk, opt) 0)
-goto cleanup;
-virBufferAddChar(opt, ',');
-} else {
-virBufferEscape(opt, ',', ,, file=%s,,
Re: [libvirt] [PATCHv1.5 04/27] qemuxml2argv: Add test for disk type='volume' with iSCSI pools
On 27/11/13 00:48, Peter Krempa wrote:
Tweak the existing file so that it can be tested for command line
corectness.
It's actually lots of change, should provide detailed commit log.
---
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c| 76 +---
src/qemu/qemu_conf.c | 129 ++---
src/qemu/qemu_conf.h | 2 +
.../qemuxml2argv-disk-source-pool-mode.args| 10 ++
.../qemuxml2argv-disk-source-pool-mode.xml | 4 +-
tests/qemuxml2argvtest.c | 2 +
8 files changed, 112 insertions(+), 113 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.args
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4561ccc..a5ef2ca 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -686,6 +686,7 @@ struct _virDomainDiskSourcePoolDef {
char *volume; /* volume name */
int voltype; /* enum virStorageVolType, internal only */
int pooltype; /* enum virStoragePoolType, internal only */
+int actualtype; /* enum virDomainDiskType, internal only */
int mode; /* enum virDomainDiskSourcePoolMode */
};
typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 205fe56..aeb3568 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -693,6 +693,7 @@ virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
+virStoragePoolTypeToString;
virStorageVolDefFindByKey;
virStorageVolDefFindByName;
virStorageVolDefFindByPath;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..b8129a3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3775,69 +3775,6 @@ qemuBuildNBDString(virConnectPtr conn,
virDomainDiskDefPtr disk, virBufferPtr op
return 0;
}
-static int
-qemuBuildVolumeString(virConnectPtr conn,
- virDomainDiskDefPtr disk,
- virBufferPtr opt)
Wondering why this is completely removed, before going ahead...
-{
-int ret = -1;
-
-switch ((virStorageVolType) disk-srcpool-voltype) {
-case VIR_STORAGE_VOL_DIR:
-if (!disk-readonly) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
- _(cannot create virtual FAT disks in read-write
mode));
-goto cleanup;
-}
-if (disk-device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
-virBufferEscape(opt, ',', ,, file=fat:floppy:%s,, disk-src);
-else
-virBufferEscape(opt, ',', ,, file=fat:%s,, disk-src);
-break;
-case VIR_STORAGE_VOL_BLOCK:
-if (disk-tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
- _(tray status 'open' is invalid for
- block type volume));
With this removed, I guess one will see different error like:
_(tray status 'open' is invalid for block type disk));
I'm doubting it's what we want for a volume type disk.
-goto cleanup;
-}
-if (disk-srcpool-pooltype == VIR_STORAGE_POOL_ISCSI) {
-if (disk-srcpool-mode ==
-VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) {
-if (qemuBuildISCSIString(conn, disk, opt) 0)
-goto cleanup;
-virBufferAddChar(opt, ',');
-} else if (disk-srcpool-mode ==
- VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) {
-virBufferEscape(opt, ',', ,, file=%s,, disk-src);
Especially here, I'm wondering if the mode attribute for iscsi pool
disk still works.
-}
-} else {
-virBufferEscape(opt, ',', ,, file=%s,, disk-src);
-}
-break;
-case VIR_STORAGE_VOL_FILE:
-if (disk-auth.username) {
-if (qemuBuildISCSIString(conn, disk, opt) 0)
-goto cleanup;
-virBufferAddChar(opt, ',');
-} else {
-virBufferEscape(opt, ',', ,, file=%s,, disk-src);
-}
-break;
-case VIR_STORAGE_VOL_NETWORK:
-case VIR_STORAGE_VOL_NETDIR:
-case VIR_STORAGE_VOL_LAST:
-/* Keep the compiler quiet, qemuTranslateDiskSourcePool already
- * reported the unsupported error.
- */
-goto cleanup;
-}
-
-ret = 0;
-
-cleanup:
-return ret;
-}
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -3851,6 +3788,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskGeometryTransTypeToString(disk-geometry.trans);
int idx = virDiskNameToIndex(disk-dst);
int busid = -1, unitid = -1;
+int
Re: [libvirt] [PATCH] libvirt-perl: Fix the wrong binding of virNodeDeviceLookupSCSIHostByWWN
On 27/11/13 17:29, Daniel P. Berrange wrote:
On Wed, Nov 27, 2013 at 03:04:26PM +0800, Osier Yang wrote:
The second parameter for virNodeDeviceLookupSCSIHostByWWN is wwnn
instead of wwpn, and the API supports flags.
---
AUTHORS| 1 +
lib/Sys/Virt/NodeDevice.pm | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index d53f191..c2af49a 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -12,5 +12,6 @@ Patches contributed by:
Stepan Kasal skasal-at-redhat-dot-com
Ludwig Nussel ludwig-dot-nussel-at-suse-dot-de
Zhe Peng zpeng-at-redhat-dot-com
+ Osier Yangjyang-at-redhat-dot-com
-- End
diff --git a/lib/Sys/Virt/NodeDevice.pm b/lib/Sys/Virt/NodeDevice.pm
index 984910d..29ceac7 100644
--- a/lib/Sys/Virt/NodeDevice.pm
+++ b/lib/Sys/Virt/NodeDevice.pm
@@ -51,10 +51,11 @@ sub _new {
my $self;
if (exists $params{name}) {
$self = Sys::Virt::NodeDevice::_lookup_by_name($con, $params{name});
-} elsif (exists $params{wwpn}) {
+} elsif (exists $params{wwnn}) {
$self = Sys::Virt::NodeDevice::_lookup_scsihost_by_wwn($con,
+ $params{wwnn},
$params{wwpn},
- $params{wwnn});
+ $params{flags});
ACK if you fix the indentation to be consistent (yes, this file is using
tabs in places)
Thanks, pushed with the indention fixed.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] VirtIO-SCSI disks limitation
[[ TO libvir-list ]] Hi, Daniel, I'm going to share the thread to public list for further discussion. Hope you don't mind. On 26/11/13 02:37, Daniel Erez wrote: Hi Osier, It seems there's a limitation in libvirt that allows up to six disks in a virtio-scsi controller. I.e. when sending more than six disks, libvirt automatically creates a new controller but of type virtual LSI Logic SCSI. Is this behavior a known issue? For narrow SCSI bus, we allow 6 disks indeed. For wide SCSI bus, we allow 15 disks (not including the controller itself on unit 7). I'm doubting if we have problem on detecting if it supports wide SCSI bus though, since as far as I see from the user cases, it's always narrow SCSI bus. Shouldn't libvirt allow up to 256 disks per controller or at least create a new controller of type virtio-scsi when needed? The controller model for virtio-scsi controller is lsilogic, which we can't change simply, since it might affect the existing guests. There was the similar discussion in libvir-list before [1]. But auto generation for controller is quite old, which I'm also not quite clear about. I'd like see another discussion to make it more clear whether we should do some work for upper layer app (e.g. oVirt). Basicly two points: * Should we do some changes on the maximum units for a SCSI controller, I.e. Should 7 (narrow bus) 16 (wide bus) be changed to other numbers? I'm afraid the changes could affect existing guests though. * Do we really want to put the burden on users, I.E, let them create the controller explicitly. For use cases like one wants to add many disks for a guest, they need to know whether it's narrow SCSI bus or wide SCSI bus first (which we don't expose outside), and then do the calculation to know when to create a new SCSI controller. @Daniel, am I correct on your problems? Please comments if it doesn't cover all your thoughts. [1] http://www.redhat.com/archives/libvir-list/2012-November/msg00537.html Regards, Osier [the issue as been discussed as part of: http://gerrit.ovirt.org/#/c/20630] Thanks, Daniel - Original Message - From: Dave Allan dal...@redhat.com To: Daniel Erez de...@redhat.com Cc: Ayal Baron aba...@redhat.com, Osier Yang jy...@redhat.com, John Ferlan jfer...@redhat.com Sent: Monday, November 25, 2013 8:19:42 PM Subject: Re: VirtIO-SCSI disks limitation Hi Daniel, Talk to Osier Yang and John Ferlan (cc'd). Dave On Mon, Nov 25, 2013 at 12:48:45PM -0500, Daniel Erez wrote: Hi Dave, I'm an engineer at oVirt team and I'm working on VirtIO-SCSI integration. I would appreciate it if you could refer me to a point of contact at libvirt. In specific, I need to know if there's any hardcoded limitation for the number of disks per VirtIO-SCSI controller. Best Regards, Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix three minor typos
On 26/11/13 15:15, Yuri Chornoivan wrote: -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list ACK and pushed. Thanks for the patch, but please consider posting the patch with git-email. Regards, Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Memory Leak Fix: Check def-info-alias before assigning
On 27/11/13 06:31, Nehal J Wani wrote:
On running the command make -C tests valgrind, there used to be a bunch of
memory leaks shown by valgrind. Specifically, one can check it by running:
libtool --mode=execute valgrind --quiet --leak-check=full
--suppressions=./.valgrind.supp qemuhotplugtest
The issue was that def-info-alias was already malloc'ed by xmlStrndup in
virDomainDeviceInfoParseXML (domain_conf.c:3439). The new alias was being
assigned again without freeing the old one in qemuAssignDeviceAliases().
This patch checks if the entity exists, and frees accordingly, hence making
valgrind cry lesser.
---
src/qemu/qemu_command.c | 39 +++
1 file changed, 39 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..bbec1d4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -979,6 +979,8 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr
qemuCaps)
size_t i;
for (i = 0; i def-ndisks; i++) {
+if (def-disks[i]-info.alias)
+VIR_FREE(def-disks[i]-info.alias);
Instead of free'ing it, it should be used to avoid calculate/strdup the
alias again. Since the alias from virDomainDeviceInfoParseXML only
works for active domain's XML:
snip
if (alias == NULL
!(flags VIR_DOMAIN_XML_INACTIVE)
xmlStrEqual(cur-name, BAD_CAST alias)) {
alias = cur;
/snip
And I don't think using the alias existing in the XML could cause any
conflicts for an active domain.
Regards,
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: skip selinux cleanup when fd not available
On 27/11/13 12:00, Eric Blake wrote:
When attempting to backport gluster pools to an older version
where there is no VIR_STRDUP, I got a crash from calling
strdup(,NULL). Rather than relying on the current else branch
safely doing nothing when there is no fd, it is easier to just
skip it. While at it, there's no need to explicitly set
perms.label to NULL after a VIR_FREE().
* src/storage/storage_backend.c
(virStorageBackendUpdateVolTargetInfoFD): Minor optimization.
Signed-off-by: Eric Blake ebl...@redhat.com
---
src/storage/storage_backend.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index bde39d6..b08d646 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1383,28 +1383,26 @@
virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
VIR_FREE(target-perms.label);
#if WITH_SELINUX
/* XXX: make this a security driver call */
-if (fd = 0 fgetfilecon_raw(fd, filecon) == -1) {
-if (errno != ENODATA errno != ENOTSUP) {
-virReportSystemError(errno,
- _(cannot get file context of '%s'),
- target-path);
-return -1;
+if (fd = 0) {
+if (fgetfilecon_raw(fd, filecon) == -1) {
+if (errno != ENODATA errno != ENOTSUP) {
+virReportSystemError(errno,
+ _(cannot get file context of '%s'),
+ target-path);
+return -1;
+}
} else {
-target-perms.label = NULL;
-}
-} else {
-if (VIR_STRDUP(target-perms.label, filecon) 0) {
+if (VIR_STRDUP(target-perms.label, filecon) 0) {
+freecon(filecon);
+return -1;
+}
freecon(filecon);
-return -1;
}
-freecon(filecon);
}
-#else
-target-perms.label = NULL;
#endif
return 0;
}
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libvirt-perl: Fix the wrong binding of virNodeDeviceLookupSCSIHostByWWN
The second parameter for virNodeDeviceLookupSCSIHostByWWN is wwnn
instead of wwpn, and the API supports flags.
---
AUTHORS| 1 +
lib/Sys/Virt/NodeDevice.pm | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index d53f191..c2af49a 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -12,5 +12,6 @@ Patches contributed by:
Stepan Kasal skasal-at-redhat-dot-com
Ludwig Nussel ludwig-dot-nussel-at-suse-dot-de
Zhe Peng zpeng-at-redhat-dot-com
+ Osier Yangjyang-at-redhat-dot-com
-- End
diff --git a/lib/Sys/Virt/NodeDevice.pm b/lib/Sys/Virt/NodeDevice.pm
index 984910d..29ceac7 100644
--- a/lib/Sys/Virt/NodeDevice.pm
+++ b/lib/Sys/Virt/NodeDevice.pm
@@ -51,10 +51,11 @@ sub _new {
my $self;
if (exists $params{name}) {
$self = Sys::Virt::NodeDevice::_lookup_by_name($con, $params{name});
-} elsif (exists $params{wwpn}) {
+} elsif (exists $params{wwnn}) {
$self = Sys::Virt::NodeDevice::_lookup_scsihost_by_wwn($con,
+ $params{wwnn},
$params{wwpn},
- $params{wwnn});
+ $params{flags});
} elsif (exists $params{xml}) {
$self = Sys::Virt::NodeDevice::_create_xml($con, $params{xml});
} else {
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv1.5 02/27] test: Implement fake storage pool driver in qemuxml2argv test
On 27/11/13 00:48, Peter Krempa wrote:
To support testing of volume disk backing, we need to implement a few
disk driver backend functions.
The fake storage driver uses files in storagepoolxml2xmlout/POOLNAME.xml
as XML files for pool definitions and volume names are in format
VOL_TYPE+VOL_PATH. By default type block is assumed (for iSCSI test
compatibility).
The choice of this approach along with implemented functions was made so
that disk type='volume' can be tested in the xml2argv test.
---
tests/qemuxml2argvtest.c | 162 +++
1 file changed, 162 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a290062..a4cef84 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -18,6 +18,7 @@
# include qemu/qemu_command.h
# include qemu/qemu_domain.h
# include datatypes.h
+# include conf/storage_conf.h
# include cpu/cpu_map.h
# include virstring.h
@@ -75,6 +76,161 @@ static virSecretDriver fakeSecretDriver = {
.secretUndefine = NULL,
};
+
+# define STORAGE_POOL_XML_PATH storagepoolxml2xmlout/
This will cause build failure when building with VPATH.
+static const unsigned char fakeUUID[VIR_UUID_BUFLEN] = fakeuuid;
+
+static virStoragePoolPtr
+fakeStoragePoolLookupByName(virConnectPtr conn,
+const char *name)
+{
+char *xmlpath = NULL;
+virStoragePoolPtr ret = NULL;
+
+if (STRNEQ(name, inactive)) {
+if (virAsprintf(xmlpath, %s%s.xml,
+STORAGE_POOL_XML_PATH,
+name) 0)
+return NULL;
+
+if (!virFileExists(xmlpath)) {
+virReportError(VIR_ERR_NO_STORAGE_POOL,
+ File '%s' not found, xmlpath);
+goto cleanup;
+}
+}
+
+ret = virGetStoragePool(conn, name, fakeUUID, NULL, NULL);
Looks like fakeUUID is only used here, so generating a random UUID
might work.
+
+cleanup:
+VIR_FREE(xmlpath);
+return ret;
+}
+
+
+static virStorageVolPtr
+fakeStorageVolLookupByName(virStoragePoolPtr pool,
+ const char *name)
+{
+char **volinfo = NULL;
+virStorageVolPtr ret = NULL;
+
+if (STREQ(pool-name, inactive)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ storage pool '%s' is not active, pool-name);
+return NULL;
+}
+
+if (STREQ(name, nonexistent)) {
It will be better if it has document what these magic strings mean.
+virReportError(VIR_ERR_NO_STORAGE_VOL,
+ no storage vol with matching name '%s', name);
+return NULL;
+}
+
+if (!strchr(name, '+'))
+goto fallback;
+
+if (!(volinfo = virStringSplit(name, +, 2)))
+return NULL;
+
+if (!volinfo[1])
+goto fallback;
+
+ret = virGetStorageVol(pool-conn, pool-name, volinfo[1], volinfo[0],
+ NULL, NULL);
+
+cleanup:
+virStringFreeList(volinfo);
+return ret;
+
+fallback:
+ret = virGetStorageVol(pool-conn, pool-name, name, block, NULL, NULL);
+goto cleanup;
+}
+
+static int
+fakeStorageVolGetInfo(virStorageVolPtr vol,
+ virStorageVolInfoPtr info)
+{
+memset(info, 0, sizeof(*info));
+
+info-type = virStorageVolTypeFromString(vol-key);
+
+if (info-type 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ Invalid volume type '%s', vol-key);
+return -1;
+}
+
+return 0;
+}
+
+
+static char *
+fakeStorageVolGetPath(virStorageVolPtr vol)
+{
+char *ret = NULL;
+
+ignore_value(virAsprintf(ret, /some/%s/device/%s, vol-key, vol-name));
+
+return ret;
+}
+
+
+static char *
+fakeStoragePoolDumpXML(virStoragePoolPtr pool,
Better to rename it as fakeStoragePoolGetXMLDesc to keep consistent.
DumpXML is used in virsh.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] storage: allow interleave in volume XML
On 23/11/13 03:54, Eric Blake wrote: The RNG grammar did not allow arbitrary interleaving, which makes it harder than necessary to create a new volume from handwritten XML. (Compare also to commit caf516db for pools). * docs/schemas/storagevol.rng: Support interleaving. * tests/storagevolxml2xmlin/vol-file-backing.xml: Test it. Signed-off-by: Eric Blake ebl...@redhat.com --- v2: correct version (actually applies to libvirt.git, rather than depending on unpublished intermediate commits) reviewer's note: see bottom for easier-to-review schema change docs/schemas/storagevol.rng| 138 + tests/storagevolxml2xmlin/vol-file-backing.xml | 15 +-- 2 files changed, 82 insertions(+), 71 deletions(-) diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 5da8e1f..e79bc35 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -13,55 +13,61 @@ define name='vol' element name='volume' - element name='name' -ref name='volName'/ - /element - optional -element name='key' - text/ + interleave +element name='name' + ref name='volName'/ /element - /optional - optional -ref name='source'/ - /optional - ref name='sizing'/ - ref name='target'/ - optional -ref name='backingStore'/ - /optional +optional + element name='key' +text/ + /element +/optional +optional + ref name='source'/ +/optional +ref name='sizing'/ +ref name='target'/ +optional + ref name='backingStore'/ +/optional + /interleave /element /define define name='sizing' -optional - element name='capacity' -ref name='scaledInteger'/ - /element -/optional -optional - element name='allocation' -ref name='scaledInteger'/ - /element -/optional +interleave + optional +element name='capacity' + ref name='scaledInteger'/ +/element + /optional + optional +element name='allocation' + ref name='scaledInteger'/ +/element + /optional +/interleave /define define name='permissions' optional element name='permissions' -element name='mode' - ref name='octalMode'/ -/element -element name='owner' - ref name='unsignedInt'/ -/element -element name='group' - ref name='unsignedInt'/ -/element -optional - element name='label' -text/ -/element -/optional +interleave + element name='mode' +ref name='octalMode'/ + /element + element name='owner' +ref name='unsignedInt'/ + /element + element name='group' +ref name='unsignedInt'/ + /element + optional +element name='label' + text/ +/element + /optional +/interleave /element /optional /define @@ -103,36 +109,40 @@ define name='target' element name='target' - optional -element name='path' - choice -data type='anyURI'/ -ref name='absFilePath'/ - /choice -/element - /optional - ref name='format'/ - ref name='permissions'/ - ref name='timestamps'/ - optional -ref name='encryption'/ - /optional - optional -ref name='compat'/ - /optional - optional -ref name='fileFormatFeatures'/ - /optional + interleave +optional + element name='path' +choice + data type='anyURI'/ + ref name='absFilePath'/ +/choice + /element +/optional +ref name='format'/ +ref name='permissions'/ +ref name='timestamps'/ +optional + ref name='encryption'/ +/optional +optional + ref name='compat'/ +/optional +optional + ref name='fileFormatFeatures'/ +/optional + /interleave /element /define define name='backingStore' element name='backingStore' - element name='path' -ref name='absFilePath'/ - /element - ref name='format'/ - ref name='permissions'/ + interleave +element name='path' + ref name='absFilePath'/ +/element +ref name='format'/ +ref name='permissions'/ + /interleave /element /define diff --git a/tests/storagevolxml2xmlin/vol-file-backing.xml b/tests/storagevolxml2xmlin/vol-file-backing.xml index 73e7f28..8610ea4 100644 --- a/tests/storagevolxml2xmlin/vol-file-backing.xml +++ b/tests/storagevolxml2xmlin/vol-file-backing.xml @@ -1,25
Re: [libvirt] [PATCH] storage: expose volume meta-type in XML
On 23/11/13 06:26, Eric Blake wrote: I got annoyed at having to use both 'virsh vol-list $pool --details' AND 'virsh vol-dumpxml $vol $pool' to learn if I had populated the volume correctly. Since two-thirds of the data present in virStorageVolGetInfo() already appears in virStorageVolGetXMLDesc(), this just adds the remaining piece of information. * docs/formatstorage.html.in: Document new target type= I didn't see it relates with target. * docs/schemas/storagevol.rng (target, backingStore): Add it to RelaxNG. I thought (target, backingStore) means add type to both of them. Finally see it means between :-) * src/conf/storage_conf.h (virStorageVolTypeToString): Declare. * src/conf/storage_conf.c (virStorageVolTargetDefFormat): Output the metatype. * tests/storagevolxml2xmlout/vol-*.xml: Update tests to match. Signed-off-by: Eric Blake ebl...@redhat.com --- Depends on: https://www.redhat.com/archives/libvir-list/2013-November/msg00948.html docs/formatstorage.html.in | 5 + docs/schemas/storagevol.rng| 15 +++ src/conf/storage_conf.c| 18 ++ src/conf/storage_conf.h| 1 + tests/storagevolxml2xmlin/vol-logical-backing.xml | 1 + tests/storagevolxml2xmlin/vol-logical.xml | 1 + tests/storagevolxml2xmlin/vol-partition.xml| 1 + tests/storagevolxml2xmlin/vol-sheepdog.xml | 1 + tests/storagevolxml2xmlout/vol-file-backing.xml| 1 + tests/storagevolxml2xmlout/vol-file-naming.xml | 1 + tests/storagevolxml2xmlout/vol-file.xml| 1 + tests/storagevolxml2xmlout/vol-logical-backing.xml | 1 + tests/storagevolxml2xmlout/vol-logical.xml | 1 + tests/storagevolxml2xmlout/vol-partition.xml | 1 + tests/storagevolxml2xmlout/vol-qcow2-0.10-lazy.xml | 1 + tests/storagevolxml2xmlout/vol-qcow2-1.1.xml | 1 + tests/storagevolxml2xmlout/vol-qcow2-lazy.xml | 1 + tests/storagevolxml2xmlout/vol-qcow2-nobacking.xml | 1 + tests/storagevolxml2xmlout/vol-qcow2.xml | 1 + tests/storagevolxml2xmlout/vol-sheepdog.xml| 1 + 20 files changed, 55 insertions(+) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 90eeaa3..5f277b4 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -293,6 +293,7 @@ lt;volumegt; lt;namegt;sparse.imglt;/namegt; lt;keygt;/var/lib/xen/images/sparse.imglt;/keygt; +lt;typegt;filelt;/typegt; lt;allocationgt;0lt;/allocationgt; lt;capacity unit=Tgt;1lt;/capacitygt; .../pre @@ -305,6 +306,10 @@ ddProviding an identifier for the volume which is globally unique. This cannot be set when creating a volume: it is always generated. span class=sinceSince 0.4.1/span/dd + dtcodetype/code/dt + ddOutput-only; provides the volume type that is also available +from codevirStorageVolGetInfo()/code. span class=sinceSince I think it's better to mention virsh vol-list $pool --details instead of the API name here, as we did across the documents. I'm fine if you keep it though. +1.1.5/span/dd dtcodeallocation/code/dt ddProviding the total storage allocation for the volume. This may be smaller than the logical capacity if the volume is sparsely diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index e79bc35..96572c5 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -25,6 +25,9 @@ optional ref name='source'/ /optional +optional + ref name='voltype'/ +/optional ref name='sizing'/ ref name='target'/ optional @@ -34,6 +37,18 @@ /element /define + define name='voltype' +element name='type' + choice +valuefile/value +valueblock/value +valuedir/value +valuenetwork/value +valuenetwork-dir/value What's network-dir type? the type you will introduce in the glusterfs series? It's not in the git head yet. So either you will need to remove it, or push this patch after the glusterfs series. + /choice +/element + /define + define name='sizing' interleave optional diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b378c2..0d2932b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -51,6 +51,10 @@ #define DEFAULT_POOL_PERM_MODE 0755 #define DEFAULT_VOL_PERM_MODE 0600 +VIR_ENUM_IMPL(virStorageVol, + VIR_STORAGE_VOL_LAST, + file, block, dir, network) Here the network-dir type is not included though. So I guess you want to push this patch before the glusterfs series. ACK if the network-dir is removed. -- libvir-list mailing list libvir-list@redhat.com
Re: [libvirt] [PATCH]lxc: don't do duplicate work when getting pagesize
On 18/11/13 16:03, Chen Hanxiao wrote:
From: Chen Hanxiao chenhanx...@cn.fujitsu.com
Don't do duplicate work when getting pagesize.
With some debug logs added.
Signed-off-by: Chen Hanxiao chenhanx...@cn.fujitsu.com
---
src/lxc/lxc_container.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 255c711..e85d01c 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -144,6 +144,7 @@ int lxcContainerHasReboot(void)
int cmd, v;
int status;
char *tmp;
+int stacksize = getpagesize() * 4;
if (virFileReadAll(/proc/sys/kernel/ctrl-alt-del, 10, buf) 0)
return -1;
@@ -160,10 +161,12 @@ int lxcContainerHasReboot(void)
VIR_FREE(buf);
cmd = v ? LINUX_REBOOT_CMD_CAD_ON : LINUX_REBOOT_CMD_CAD_OFF;
-if (VIR_ALLOC_N(stack, getpagesize() * 4) 0)
+if (VIR_ALLOC_N(stack, stacksize) 0) {
+VIR_DEBUG(Unable to allocate stack);
virAllocN already reports the out-of-memory error, why do we
need a debug log?
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv4 0/8] glusterfs storage pool
On 23/11/13 11:20, Eric Blake wrote: v3: https://www.redhat.com/archives/libvir-list/2013-November/msg00348.html Depends on: https://www.redhat.com/archives/libvir-list/2013-November/msg00955.html Changes since then, addressing review feedback: - rebase to other improvements in the meantime - New patches 4-7 - pool changed to require namevolume/name to have no slash, with subdirectory within a volume selected by dir path=.../ which must begin with slash - documentation improved to match actual testing - directories, symlinks are handled - volume owner and timestamps are handled - volume xml tests added, with several bugs in earlier version fixed along the way - compared gluster pool with a netfs pool to ensure both can see the same level of detail from the same gluster storage If you think it will help review, ask me to provide an interdiff from v3 (although I have not done it yet). Eric Blake (8): storage: initial support for linking with libgfapi storage: document gluster pool storage: implement rudimentary glusterfs pool refresh storage: add network-dir as new storage volume type storage: improve directory support in gluster pool storage: improve allocation stats reported on gluster files storage: improve handling of symlinks in gluster storage: probe qcow2 volumes in gluster pool Looked through the whole set, except the version nit pointed out by Daniel, I didn't see any other problem. So ACK. /btw, I need to support the gluster pool for domain config after these patches are pushed. Regards, Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Doc: Explicitly declaring that nodedev-destroy only works for vHBA
Though trying to destroy a physical HBA doesn't make sense at all, it's still a bit misleading with saying only works for HBA. Signed-off-by: Osier Yang jy...@redhat.com --- src/libvirt.c | 5 +++-- tools/virsh.pod | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ae05300..4ebd13f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16065,8 +16065,9 @@ error: * virNodeDeviceDestroy: * @dev: a device object * - * Destroy the device object. The virtual device is removed from the host operating system. - * This function may require privileged access + * Destroy the device object. The virtual device (only works for vHBA + * currently) is removed from the host operating system. This function + * may require privileged access. * * Returns 0 in case of success and -1 in case of failure. */ diff --git a/tools/virsh.pod b/tools/virsh.pod index dac9a08..557df9c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2158,9 +2158,9 @@ contains xml for a top-level device description of a node device. =item Bnodedev-destroy Idevice Destroy (stop) a device on the host. Idevice can be either device -name or wwn pair in wwnn,wwpn format (only works for HBA). Note -that this makes libvirt quit managing a host device, and may even make -that device unusable by the rest of the physical host until a reboot. +name or wwn pair in wwnn,wwpn format (only works for vHBA currently). +Note that this makes libvirt quit managing a host device, and may even +make that device unusable by the rest of the physical host until a reboot. =item Bnodedev-detach Inodedev [I--driver backend_driver] -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage: Returns earlier if source adapter of the scsi pool is a HBA
It makes no sense to go forward to get the parent host number of a HBA, and treat the HBA as a vHBA with trying to delete it. --- src/storage/storage_backend_scsi.c | 8 1 file changed, 8 insertions(+) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 1426121..6f86ffc 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -667,6 +667,14 @@ deleteVport(virStoragePoolSourceAdapter adapter) if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) return 0; +/* It must be a HBA instead of a vHBA as long as parent + * is NULL. createVport guaranteed parent for a vHBA + * cannot be NULL, it's either specified in XML, or detected + * automatically. + */ +if (!adapter.data.fchost.parent) +return 0; + if (!(virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn, adapter.data.fchost.wwpn))) return -1; -- 1.8.1.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/11] maint: fix comma style issues: nwfilter
On 20/11/13 08:30, Eric Blake wrote: Most of our code base uses space after comma but not before; fix the remaining uses before adding a syntax check. * src/nwfilter/nwfilter_ebiptables_driver.c: Consistently use commas. * src/nwfilter/nwfilter_gentech_driver.c: Likewise. * src/nwfilter/nwfilter_learnipaddr.c: Likewise. * src/conf/nwfilter_conf.c: Likewise. Signed-off-by: Eric Blake ebl...@redhat.com --- src/conf/nwfilter_conf.c | 92 --- src/nwfilter/nwfilter_ebiptables_driver.c | 40 +++--- src/nwfilter/nwfilter_gentech_driver.c| 2 +- src/nwfilter/nwfilter_learnipaddr.c | 2 +- 4 files changed, 70 insertions(+), 66 deletions(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
