April 2014 - Devel - libvirt List Archives

[libvirt] [PATCH] storage: netfs: Handle backend errors
by John Ferlan 15 Apr '14

15 Apr '14

Commit id '18642d10' caused a virt-test regression for NFS backend storage error path checks when running the command: 'virsh find-storage-pool-sources-as netfs Unknown ' when the host did not have Gluster installed. Prior to the commit, the test would fail with the error: error: internal error: Child process (/usr/sbin/showmount --no-headers --exports Unknown) unexpected exit status 1: clnt_create: RPC: Unknown host After the commit, the error would be ignored, the call would succeed, and an empty list of pool sources returned. This was tucked into the commit message as an expected outcome. When the target host does not have a GLUSTER_CLI this is a regression over the previous release. Furthermore, even if Gluster CLI was present, but had a failure to get devices, the API would return a failure even if the NFS backend had found devices. Add code to handle the error conditions. - If NFS fails - If there is no Gluster CLI, then jump to cleanup/failure. - If there is a Gluster CLI, then message the NFS failure and continue with the Gluster checks. - If Gluster fails - If NFS had failed, then jump to cleanup/failure. - Message the Gluster failure and continue on. - If only one fails, fetch and return a list of source devices even if it's empty Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/storage/storage_backend.c | 14 +++++++++++++ src/storage/storage_backend.h | 1 + src/storage/storage_backend_fs.c | 43 +++++++++++++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b56fefe..4d73902 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1643,6 +1643,13 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, } #ifdef GLUSTER_CLI +bool +virStorageBackendHaveGlusterCLI(void) +{ + return true; +} + + int virStorageBackendFindGlusterPoolSources(const char *host, int pooltype, @@ -1721,6 +1728,13 @@ virStorageBackendFindGlusterPoolSources(const char *host, return ret; } #else /* #ifdef GLUSTER_CLI */ +bool +virStorageBackendHaveGlusterCLI(void) +{ + return false; +} + + int virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED, int pooltype ATTRIBUTE_UNUSED, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 4f95000..41eed97 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -87,6 +87,7 @@ int virStorageBackendFindFSImageTool(char **tool); virStorageBackendBuildVolFrom virStorageBackendFSImageToolTypeToFunc(int tool_type); +bool virStorageBackendHaveGlusterCLI(void); int virStorageBackendFindGlusterPoolSources(const char *host, int pooltype, virStoragePoolSourceListPtr list); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4e4a7ae..f3d4a6d 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -238,9 +238,11 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(char **const groups, } -static void +static int virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state) { + int ret = -1; + /* * # showmount --no-headers -e HOSTNAME * /tmp * @@ -267,9 +269,13 @@ virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state) if (virCommandRunRegex(cmd, 1, regexes, vars, virStorageBackendFileSystemNetFindPoolSourcesFunc, state, NULL) < 0) - virResetLastError(); + goto cleanup; + ret = 0; + + cleanup: virCommandFree(cmd); + return ret; } @@ -289,6 +295,7 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE virStoragePoolSourcePtr source = NULL; char *ret = NULL; size_t i; + bool failNFS = false; virCheckFlags(0, NULL); @@ -310,12 +317,38 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE state.host = source->hosts[0].name; - virStorageBackendFileSystemNetFindNFSPoolSources(&state); + if (virStorageBackendFileSystemNetFindNFSPoolSources(&state) < 0) { + virErrorPtr err; + /* If no Gluster CLI, then force error right here */ + if (!virStorageBackendHaveGlusterCLI()) + goto cleanup; + + /* If we have a Gluster CLI, then message the error, clean it out, + * and move onto the Gluster code + */ + err = virGetLastError(); + VIR_ERROR(_("Failed to get NFS pool sources: '%s'"), + err != NULL ? err->message: _("unknown error")); + virResetLastError(); + failNFS = true; + } if (virStorageBackendFindGlusterPoolSources(state.host, VIR_STORAGE_POOL_NETFS_GLUSTERFS, - &state.list) < 0) - goto cleanup; + &state.list) < 0) { + virErrorPtr err; + /* If NFS failed as well, then force the error right here */ + if (failNFS) + goto cleanup; + + /* Otherwise, NFS passed, so we message the Gluster error, clean + * it out, and generate the source list (even if it's empty) + */ + err = virGetLastError(); + VIR_ERROR(_("Failed to get Gluster pool sources: '%s'"), + err != NULL ? err->message: _("unknown error")); + virResetLastError(); + } if (!(ret = virStoragePoolSourceListFormat(&state.list))) goto cleanup; -- 1.9.0

2 3

[libvirt] [PATCH] Fix virsystemdtest without SYSTEMD_DAEMON
by Ján Tomko 15 Apr '14

15 Apr '14

Commit 4897698 fixed the build without dbus by only building the virSystemdPMSupportTarget with SYSTEMD_DAEMON. Introduce a virDBusMessageUnref wrapper for dbus_message_unref to let virsystemd.c build without dbus, while still allowing virsystemdtest to run without SYSTEM_DAEMON. --- src/util/virdbus.c | 9 +++++++++ src/util/virdbus.h | 1 + src/util/virsystemd.c | 11 +---------- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 0cd3858..4217aea 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1614,6 +1614,11 @@ int virDBusIsServiceRegistered(const char *name) return ret; } +void virDBusMessageUnref(DBusMessage *msg) +{ + dbus_message_unref(msg); +} + #else /* ! WITH_DBUS */ void virDBusSetSharedBus(bool shared ATTRIBUTE_UNUSED) { @@ -1751,4 +1756,8 @@ int virDBusIsServiceRegistered(const char *name ATTRIBUTE_UNUSED) return -2; } +void virDBusMessageUnref(DBusMessage *msg ATTRIBUTE_UNUSED) +{ + /* nothing */ +} #endif /* ! WITH_DBUS */ diff --git a/src/util/virdbus.h b/src/util/virdbus.h index 0f21821..a4987b8 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -74,6 +74,7 @@ int virDBusCall(DBusConnection *conn, DBusError *error); int virDBusMessageRead(DBusMessage *msg, const char *types, ...); +void virDBusMessageUnref(DBusMessage *msg); int virDBusIsServiceEnabled(const char *name); int virDBusIsServiceRegistered(const char *name); diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index e67956f..ad15a18 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -326,7 +326,6 @@ virSystemdNotifyStartup(void) #endif } -#ifdef WITH_SYSTEMD_DAEMON static int virSystemdPMSupportTarget(const char *methodName, bool *result) { @@ -365,19 +364,11 @@ virSystemdPMSupportTarget(const char *methodName, bool *result) ret = 0; cleanup: - dbus_message_unref(message); + virDBusMessageUnref(message); VIR_FREE(response); return ret; } -#else /* ! WITH_SYSTEMD_DAEMON */ -static int -virSystemdPMSupportTarget(const char *methodName ATTRIBUTE_UNUSED, - bool *result ATTRIBUTE_UNUSED) -{ - return -2; -} -#endif /* ! WITH_SYSTEMD_DAEMON */ int virSystemdCanSuspend(bool *result) { -- 1.8.3.2

2 2

[libvirt] Java Bindings ConnectAuthDefault callback never called
by Sergio Rodriguez de Guzman 14 Apr '14

14 Apr '14

I'm following the instructions mentioned in the list some years ago about using a subclass of ConnectAuth using ConnectAuthDefault as a sample: "You could create your own subclass of ConnectAuth, and override the method:" public abstract int callback(Credential[] cred); What I'm doing is just "copying" ConnectAuthDefault to ConnectAuthAlternate and using exactly the same code. The thing is that the callback defined in that class never fires: ConnectAuth defaultAuth = new ConnectAuthAlternate(); try{ conn = new Connect("qemu+ssh://student@10.164.1.21/system", defaultAuth, 0); } catch (LibvirtException e){ System.out.println("exception caught:"+e); System.out.println(e.getError()); } It works as expected, asks for the user password and connects successfully but it never reaches the callback defined in ConnectAuthAlternate (aka ConnectAuthDefault): @Override public int callback(Credential[] cred) { BufferedReader in = new BufferedReader(new InputStreamReader(System.in)); [...] I'm using libvirt-java 0.5.1, JNA 4.1 and the current libvirt version that comes with ubuntu 13.10: 1.1.1-0ubuntu8.5 Could you give some light about this?. Sergio.

1 0

[libvirt] [PATCH 0/6] RelaxNG improvements for virStorageSource
by Eric Blake 14 Apr '14

14 Apr '14

Part of extending the domain XML to show an entire backing chain is splitting the RelaxNG definitions so that the portion related to a single source within the chain can easily be reused. I'm still debating about the way to handle storage volumes using <format type='qcow2'/> vs. domain disks using <driver name='qcow2' type='qcow2'/>. The majority of the <driver> attributes are associated with the one disk device presented to the guest, rather than attributes for a file in the chain. So I'm thinking of having domain disks add a new <format> element; where we output the format under both elements but accept either spelling for new input (back-compat reasons require that we always provide the old spelling). Eric Blake (6): conf: create common storage RNG grammar file conf: better <disk> interleaving in schema conf: tighten <domainsnapshot> schema conf: split <disk> schema into more pieces conf: move storage source details to common RNG file conf: move <auth> and <encryption> to disk source docs/schemas/Makefile.am | 5 +- docs/schemas/domain.rng | 15 +- docs/schemas/domaincommon.rng | 268 ++---------------- docs/schemas/domainsnapshot.rng | 85 +++--- docs/schemas/storagecommon.rng | 312 +++++++++++++++++++++ docs/schemas/storageencryption.rng | 33 --- docs/schemas/storagefilefeatures.rng | 24 -- docs/schemas/storagevol.rng | 3 +- .../qemuxml2argv-disk-drive-discard.xml | 8 +- .../qemuxml2argv-disk-source-pool.xml | 12 +- .../qemuxml2xmlout-disk-drive-discard.xml | 37 +++ .../qemuxml2xmlout-disk-source-pool.xml | 44 +++ tests/qemuxml2xmltest.c | 4 +- 13 files changed, 477 insertions(+), 373 deletions(-) create mode 100644 docs/schemas/storagecommon.rng delete mode 100644 docs/schemas/storageencryption.rng delete mode 100644 docs/schemas/storagefilefeatures.rng create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-discard.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml -- 1.9.0

2 9

[libvirt] [PATCHv2] virsh: Make vshCommandOpt* report error
by Michal Privoznik 14 Apr '14

14 Apr '14

Currently, the virsh code is plenty of the following pattern: if (vshCommandOptUInt(..) < 0) { vshError(...); goto cleanup; } It doesn't make much sense to repeat the code everywhere. Moreover, some functions from the family already report error some of them don't. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tests/vcpupin | 2 +- tools/virsh-domain-monitor.c | 7 +-- tools/virsh-domain.c | 102 +++++++++++++++---------------------------- tools/virsh-host.c | 25 +++-------- tools/virsh-interface.c | 4 +- tools/virsh-network.c | 4 +- tools/virsh-volume.c | 16 ++----- tools/virsh.c | 99 +++++++++++++++++++++++++++++++---------- tools/virsh.h | 24 +++++----- 9 files changed, 140 insertions(+), 143 deletions(-) diff --git a/tests/vcpupin b/tests/vcpupin index f1fb038..638f62b 100755 --- a/tests/vcpupin +++ b/tests/vcpupin @@ -34,7 +34,7 @@ fail=0 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test a 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: vcpupin: Invalid or missing vCPU number. +error: Unable to parse integer parameter to --vcpu EOF compare exp out || fail=1 diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 18d551a..0da4f91 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -324,12 +324,9 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd) /* Providing a period will adjust the balloon driver collection period. * This is not really an unsigned long, but it */ - if ((rv = vshCommandOptInt(cmd, "period", &period)) < 0) { - vshError(ctl, "%s", - _("Unable to parse integer parameter.")); + if ((rv = vshCommandOptInt(ctl, cmd, "period", &period)) < 0) { goto cleanup; - } - if (rv > 0) { + } else if (rv > 0) { if (period < 0) { vshError(ctl, _("Invalid collection period value '%d'"), period); goto cleanup; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 73414f8..821ed62 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1119,8 +1119,8 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "device", &disk) < 0) goto cleanup; - if ((rv = vshCommandOptULongLong(cmd, "total-bytes-sec", &value)) < 0) { - goto interror; + if ((rv = vshCommandOptULongLong(ctl, cmd, "total-bytes-sec", &value)) < 0) { + goto cleanup; } else if (rv > 0) { if (virTypedParamsAddULLong(&params, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, @@ -1128,8 +1128,8 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rv = vshCommandOptULongLong(cmd, "read-bytes-sec", &value)) < 0) { - goto interror; + if ((rv = vshCommandOptULongLong(ctl, cmd, "read-bytes-sec", &value)) < 0) { + goto cleanup; } else if (rv > 0) { if (virTypedParamsAddULLong(&params, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, @@ -1137,8 +1137,8 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rv = vshCommandOptULongLong(cmd, "write-bytes-sec", &value)) < 0) { - goto interror; + if ((rv = vshCommandOptULongLong(ctl, cmd, "write-bytes-sec", &value)) < 0) { + goto cleanup; } else if (rv > 0) { if (virTypedParamsAddULLong(&params, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, @@ -1146,8 +1146,8 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rv = vshCommandOptULongLong(cmd, "total-iops-sec", &value)) < 0) { - goto interror; + if ((rv = vshCommandOptULongLong(ctl, cmd, "total-iops-sec", &value)) < 0) { + goto cleanup; } else if (rv > 0) { if (virTypedParamsAddULLong(&params, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, @@ -1155,8 +1155,8 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rv = vshCommandOptULongLong(cmd, "read-iops-sec", &value)) < 0) { - goto interror; + if ((rv = vshCommandOptULongLong(ctl, cmd, "read-iops-sec", &value)) < 0) { + goto cleanup; } else if (rv > 0) { if (virTypedParamsAddULLong(&params, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, @@ -1164,8 +1164,8 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rv = vshCommandOptULongLong(cmd, "write-iops-sec", &value)) < 0) { - goto interror; + if ((rv = vshCommandOptULongLong(ctl, cmd, "write-iops-sec", &value)) < 0) { + goto cleanup; } else if (rv > 0) { if (virTypedParamsAddULLong(&params, &nparams, &maxparams, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, @@ -1216,10 +1216,6 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) error: vshError(ctl, "%s", _("Unable to change block I/O throttle")); goto cleanup; - - interror: - vshError(ctl, "%s", _("Unable to parse integer parameter")); - goto cleanup; } /* @@ -1315,8 +1311,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if ((rv = vshCommandOptInt(cmd, "weight", &weight)) < 0) { - vshError(ctl, "%s", _("Unable to parse integer parameter")); + if ((rv = vshCommandOptInt(ctl, cmd, "weight", &weight)) < 0) { goto cleanup; } else if (rv > 0) { if (weight <= 0) { @@ -1457,10 +1452,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0) { - vshError(ctl, "%s", _("bandwidth must be a number")); + if (vshCommandOptUL(ctl, cmd, "bandwidth", &bandwidth) < 0) goto cleanup; - } switch ((vshCmdBlockJobMode) mode) { case VSH_CMD_BLOCK_JOB_ABORT: @@ -2215,10 +2208,8 @@ cmdBlockResize(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", (const char **) &path) < 0) return false; - if (vshCommandOptScaledInt(cmd, "size", &size, 1024, ULLONG_MAX) < 0) { - vshError(ctl, "%s", _("Unable to parse integer")); + if (vshCommandOptScaledInt(ctl, cmd, "size", &size, 1024, ULLONG_MAX) < 0) return false; - } /* Prefer the older interface of KiB. */ if (size % 1024 == 0) @@ -2805,10 +2796,8 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - if (vshCommandOptULongLong(cmd, "duration", &duration) < 0) { - vshError(ctl, _("Invalid duration argument")); + if (vshCommandOptULongLong(ctl, cmd, "duration", &duration) < 0) goto cleanup; - } if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) goto cleanup; @@ -4709,10 +4698,8 @@ cmdScreenshot(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "file", (const char **) &file) < 0) return false; - if (vshCommandOptUInt(cmd, "screen", &screen) < 0) { - vshError(ctl, "%s", _("invalid screen ID")); + if (vshCommandOptUInt(ctl, cmd, "screen", &screen) < 0) return false; - } if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; @@ -5819,9 +5806,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) query = !cpulist; /* In query mode, "vcpu" is optional */ - if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) { - vshError(ctl, "%s", - _("vcpupin: Invalid or missing vCPU number.")); + if (vshCommandOptInt(ctl, cmd, "vcpu", &vcpu) < !query) { virDomainFree(dom); return false; } @@ -6063,7 +6048,7 @@ static bool cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - int count = 0; + unsigned int count = 0; bool ret = false; bool maximum = vshCommandOptBool(cmd, "maximum"); bool config = vshCommandOptBool(cmd, "config"); @@ -6089,10 +6074,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptInt(cmd, "count", &count) < 0 || count <= 0) { - vshError(ctl, "%s", _("Invalid number of virtual CPUs")); + if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0) goto cleanup; - } if (flags == -1) { if (virDomainSetVcpus(dom, count) != 0) @@ -6384,8 +6367,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) show_total = vshCommandOptBool(cmd, "total"); - if ((rv = vshCommandOptInt(cmd, "start", &cpu)) < 0) { - vshError(ctl, "%s", _("Unable to parse integer parameter for start")); + if ((rv = vshCommandOptInt(ctl, cmd, "start", &cpu)) < 0) { goto cleanup; } else if (rv > 0) { if (cpu < 0) { @@ -6395,9 +6377,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) show_per_cpu = true; } - if ((rv = vshCommandOptInt(cmd, "count", &show_count)) < 0) { - vshError(ctl, "%s", - _("Unable to parse integer parameter for CPUs to show")); + if ((rv = vshCommandOptInt(ctl, cmd, "count", &show_count)) < 0) { goto cleanup; } else if (rv > 0) { if (show_count < 0) { @@ -7157,10 +7137,8 @@ cmdSendKey(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "codeset", &codeset_option) <= 0) codeset_option = "linux"; - if (vshCommandOptUInt(cmd, "holdtime", &holdtime) < 0) { - vshError(ctl, _("invalid value of --holdtime")); + if (vshCommandOptUInt(ctl, cmd, "holdtime", &holdtime) < 0) goto cleanup; - } codeset = virKeycodeSetTypeFromString(codeset_option); if (codeset < 0) { @@ -7384,8 +7362,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) max = 1024ull * ULONG_MAX; else max = ULONG_MAX; - if (vshCommandOptScaledInt(cmd, "size", &bytes, 1024, max) < 0) { - vshError(ctl, "%s", _("memory size has to be a number")); + if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) { virDomainFree(dom); return false; } @@ -7481,8 +7458,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) max = 1024ull * ULONG_MAX; else max = ULONG_MAX; - if (vshCommandOptScaledInt(cmd, "size", &bytes, 1024, max) < 0) { - vshError(ctl, "%s", _("memory size has to be a number")); + if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) { virDomainFree(dom); return false; } @@ -8123,9 +8099,11 @@ cmdQemuAttach(vshControl *ctl, const vshCmd *cmd) bool ret = false; unsigned int flags = 0; unsigned int pid_value; /* API uses unsigned int, not pid_t */ + int rv; - if (vshCommandOptUInt(cmd, "pid", &pid_value) <= 0) { - vshError(ctl, "%s", _("missing pid value")); + if ((rv = vshCommandOptUInt(ctl, cmd, "pid", &pid_value)) <= 0) { + if (rv == 0) + vshError(ctl, "%s", _("missing pid value")); goto cleanup; } @@ -8220,9 +8198,8 @@ cmdQemuAgentCommand(vshControl *ctl, const vshCmd *cmd) } guest_agent_cmd = virBufferContentAndReset(&buf); - judge = vshCommandOptInt(cmd, "timeout", &timeout); + judge = vshCommandOptInt(ctl, cmd, "timeout", &timeout); if (judge < 0) { - vshError(ctl, "%s", _("timeout number has to be a number")); goto cleanup; } else if (judge > 0) { judge = 1; @@ -9089,17 +9066,14 @@ static bool cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - long long downtime = 0; + unsigned long long downtime = 0; bool ret = false; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptLongLong(cmd, "downtime", &downtime) < 0 || - downtime < 1) { - vshError(ctl, "%s", _("migrate: Invalid downtime")); + if (vshCommandOptULongLong(ctl, cmd, "downtime", &downtime) < 0) goto done; - } if (virDomainMigrateSetMaxDowntime(dom, downtime, 0)) goto done; @@ -9152,9 +9126,7 @@ cmdMigrateCompCache(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - rc = vshCommandOptULongLong(cmd, "size", &size); - if (rc < 0) { - vshError(ctl, "%s", _("Unable to parse size parameter")); + if ((rc = vshCommandOptULongLong(ctl, cmd, "size", &size)) < 0) { goto cleanup; } else if (rc != 0) { if (virDomainMigrateSetCompressionCache(dom, size, 0) < 0) @@ -9211,10 +9183,8 @@ cmdMigrateSetMaxSpeed(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0) { - vshError(ctl, "%s", _("migrate: Invalid bandwidth")); + if (vshCommandOptUL(ctl, cmd, "bandwidth", &bandwidth) < 0) goto done; - } if (virDomainMigrateSetMaxSpeed(dom, bandwidth, 0) < 0) goto done; @@ -11371,10 +11341,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return ret; - if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) { - vshError(ctl, _("Unable to parse integer parameter minimum")); + if (vshCommandOptULongLong(ctl, cmd, "minimum", &minimum) < 0) goto cleanup; - } if (vshCommandOptStringReq(ctl, cmd, "mountpoint", &mountPoint) < 0) goto cleanup; diff --git a/tools/virsh-host.c b/tools/virsh-host.c index cac6086..61be4c0 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -111,10 +111,8 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(all, cellno); - if (cellno && vshCommandOptInt(cmd, "cellno", &cell) < 0) { - vshError(ctl, "%s", _("cell number has to be a number")); + if (cellno && vshCommandOptInt(ctl, cmd, "cellno", &cell) < 0) return false; - } if (all) { if (!(cap_xml = virConnectGetCapabilities(ctl->conn))) { @@ -372,10 +370,8 @@ cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd) unsigned long long cpu_stats[VSH_CPU_LAST] = { 0 }; bool present[VSH_CPU_LAST] = { false }; - if (vshCommandOptInt(cmd, "cpu", &cpuNum) < 0) { - vshError(ctl, "%s", _("Invalid value of cpuNum")); + if (vshCommandOptInt(ctl, cmd, "cpu", &cpuNum) < 0) return false; - } if (virNodeGetCPUStats(ctl->conn, cpuNum, NULL, &nparams, 0) != 0) { vshError(ctl, "%s", @@ -481,10 +477,8 @@ cmdNodeMemStats(vshControl *ctl, const vshCmd *cmd) virNodeMemoryStatsPtr params = NULL; bool ret = false; - if (vshCommandOptInt(cmd, "cell", &cellNum) < 0) { - vshError(ctl, "%s", _("Invalid value of cellNum")); + if (vshCommandOptInt(ctl, cmd, "cell", &cellNum) < 0) return false; - } /* get the number of memory parameters */ if (virNodeGetMemoryStats(ctl->conn, cellNum, NULL, &nparams, 0) != 0) { @@ -555,10 +549,8 @@ cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) return false; - if (vshCommandOptLongLong(cmd, "duration", &duration) < 0) { - vshError(ctl, _("Invalid duration argument")); + if (vshCommandOptLongLong(ctl, cmd, "duration", &duration) < 0) return false; - } if (STREQ(target, "mem")) suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM; @@ -862,8 +854,7 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) int rc = -1; size_t i; - if ((rc = vshCommandOptUInt(cmd, "shm-pages-to-scan", &value)) < 0) { - vshError(ctl, "%s", _("invalid shm-pages-to-scan number")); + if ((rc = vshCommandOptUInt(ctl, cmd, "shm-pages-to-scan", &value)) < 0) { goto cleanup; } else if (rc > 0) { if (virTypedParamsAddUInt(&params, &nparams, &maxparams, @@ -872,8 +863,7 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rc = vshCommandOptUInt(cmd, "shm-sleep-millisecs", &value)) < 0) { - vshError(ctl, "%s", _("invalid shm-sleep-millisecs number")); + if ((rc = vshCommandOptUInt(ctl, cmd, "shm-sleep-millisecs", &value)) < 0) { goto cleanup; } else if (rc > 0) { if (virTypedParamsAddUInt(&params, &nparams, &maxparams, @@ -882,8 +872,7 @@ cmdNodeMemoryTune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if ((rc = vshCommandOptUInt(cmd, "shm-merge-across-nodes", &value)) < 0) { - vshError(ctl, "%s", _("invalid shm-merge-across-nodes number")); + if ((rc = vshCommandOptUInt(ctl, cmd, "shm-merge-across-nodes", &value)) < 0) { goto cleanup; } else if (rc > 0) { if (virTypedParamsAddUInt(&params, &nparams, &maxparams, diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index d4ec854..4dfd4ae 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -844,10 +844,8 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd *cmd) /* use "no-stp" because we want "stp" to default true */ stp = !vshCommandOptBool(cmd, "no-stp"); - if (vshCommandOptUInt(cmd, "delay", &delay) < 0) { - vshError(ctl, "%s", _("Unable to parse delay parameter")); + if (vshCommandOptUInt(ctl, cmd, "delay", &delay) < 0) goto cleanup; - } nostart = vshCommandOptBool(cmd, "no-start"); diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 4b0df62..1f42b0d 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -943,10 +943,8 @@ cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (vshCommandOptInt(cmd, "parent-index", &parentIndex) < 0) { - vshError(ctl, "%s", _("malformed parent-index argument")); + if (vshCommandOptInt(ctl, cmd, "parent-index", &parentIndex) < 0) goto cleanup; - } /* The goal is to have a full xml element in the "xml" * string. This is provided in the --xml option, either directly diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 55bf6f0..24a2b14 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -658,15 +658,11 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; unsigned long long offset = 0, length = 0; - if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) { - vshError(ctl, _("Unable to parse integer")); + if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) return false; - } - if (vshCommandOptULongLong(cmd, "length", &length) < 0) { - vshError(ctl, _("Unable to parse integer")); + if (vshCommandOptULongLong(ctl, cmd, "length", &length) < 0) return false; - } if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) { return false; @@ -768,15 +764,11 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; bool created = false; - if (vshCommandOptULongLong(cmd, "offset", &offset) < 0) { - vshError(ctl, _("Unable to parse integer")); + if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) return false; - } - if (vshCommandOptULongLong(cmd, "length", &length) < 0) { - vshError(ctl, _("Unable to parse integer")); + if (vshCommandOptULongLong(ctl, cmd, "length", &length) < 0) return false; - } if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", &name))) return false; diff --git a/tools/virsh.c b/tools/virsh.c index 28af3c3..b2edea3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1469,6 +1469,7 @@ vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt, /** * vshCommandOptInt: + * @ctl virsh control structure * @cmd command reference * @name option name * @value result @@ -1480,7 +1481,10 @@ vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt, * <0 in all other cases (@value untouched) */ int -vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) +vshCommandOptInt(vshControl *ctl, + const vshCmd *cmd, + const char *name, + int *value) { vshCmdOpt *arg; int ret; @@ -1489,14 +1493,19 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) if (ret <= 0) return ret; - if (virStrToLong_i(arg->data, NULL, 10, value) < 0) + if (virStrToLong_i(arg->data, NULL, 10, value) < 0) { + vshError(ctl, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } return 1; } /** * vshCommandOptUInt: + * @ctl virsh control structure * @cmd command reference * @name option name * @value result @@ -1505,7 +1514,10 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) * See vshCommandOptInt() */ int -vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) +vshCommandOptUInt(vshControl *ctl, + const vshCmd *cmd, + const char *name, + unsigned int *value) { vshCmdOpt *arg; int ret; @@ -1514,14 +1526,19 @@ vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) if (ret <= 0) return ret; - if (virStrToLong_ui(arg->data, NULL, 10, value) < 0) + if (virStrToLong_ui(arg->data, NULL, 10, value) < 0) { + vshError(ctl, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } return 1; } /* * vshCommandOptUL: + * @ctl virsh control structure * @cmd command reference * @name option name * @value result @@ -1530,7 +1547,10 @@ vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) * See vshCommandOptInt() */ int -vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) +vshCommandOptUL(vshControl *ctl, + const vshCmd *cmd, + const char *name, + unsigned long *value) { vshCmdOpt *arg; int ret; @@ -1539,8 +1559,12 @@ vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) if (ret <= 0) return ret; - if (virStrToLong_ul(arg->data, NULL, 10, value) < 0) + if (virStrToLong_ul(arg->data, NULL, 10, value) < 0) { + vshError(ctl, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } return 1; } @@ -1620,6 +1644,7 @@ vshCommandOptStringReq(vshControl *ctl, /** * vshCommandOptLongLong: + * @ctl virsh control structure * @cmd command reference * @name option name * @value result @@ -1628,7 +1653,9 @@ vshCommandOptStringReq(vshControl *ctl, * See vshCommandOptInt() */ int -vshCommandOptLongLong(const vshCmd *cmd, const char *name, +vshCommandOptLongLong(vshControl *ctl, + const vshCmd *cmd, + const char *name, long long *value) { vshCmdOpt *arg; @@ -1638,13 +1665,18 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, if (ret <= 0) return ret; - if (virStrToLong_ll(arg->data, NULL, 10, value) < 0) + if (virStrToLong_ll(arg->data, NULL, 10, value) < 0) { + vshError(ctl, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } return 1; } /** * vshCommandOptULongLong: + * @ctl virsh control structure * @cmd command reference * @name option name * @value result @@ -1653,7 +1685,9 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, * See vshCommandOptInt() */ int -vshCommandOptULongLong(const vshCmd *cmd, const char *name, +vshCommandOptULongLong(vshControl *ctl, + const vshCmd *cmd, + const char *name, unsigned long long *value) { vshCmdOpt *arg; @@ -1663,14 +1697,19 @@ vshCommandOptULongLong(const vshCmd *cmd, const char *name, if (ret <= 0) return ret; - if (virStrToLong_ull(arg->data, NULL, 10, value) < 0) + if (virStrToLong_ull(arg->data, NULL, 10, value) < 0) { + vshError(ctl, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } return 1; } /** * vshCommandOptScaledInt: + * @ctl virsh control structure * @cmd command reference * @name option name * @value result @@ -1681,8 +1720,11 @@ vshCommandOptULongLong(const vshCmd *cmd, const char *name, * See vshCommandOptInt() */ int -vshCommandOptScaledInt(const vshCmd *cmd, const char *name, - unsigned long long *value, int scale, +vshCommandOptScaledInt(vshControl *ctl, + const vshCmd *cmd, + const char *name, + unsigned long long *value, + int scale, unsigned long long max) { const char *str; @@ -1692,9 +1734,18 @@ vshCommandOptScaledInt(const vshCmd *cmd, const char *name, ret = vshCommandOptString(cmd, name, &str); if (ret <= 0) return ret; - if (virStrToLong_ull(str, &end, 10, value) < 0 || - virScaleInteger(value, end, scale, max) < 0) + if (virStrToLong_ull(str, &end, 10, value) < 0) { + vshError(ctl, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } + + if (virScaleInteger(value, end, scale, max) < 0) { + vshError(ctl, "%s", + _("value falls out of range")); + return -1; + } return 1; } @@ -1771,20 +1822,22 @@ vshCmdHasOption(vshControl *ctl, const vshCmd *cmd, const char *optname) int vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout) { - int rv = vshCommandOptInt(cmd, "timeout", timeout); + int rv = vshCommandOptInt(ctl, cmd, "timeout", timeout); - if (rv < 0 || (rv > 0 && *timeout < 1)) { + if (rv <= 0) + return rv; + + if (*timeout < 1) { vshError(ctl, "%s", _("invalid timeout")); return -1; } - if (rv > 0) { - /* Ensure that we can multiply by 1000 without overflowing. */ - if (*timeout > INT_MAX / 1000) { - vshError(ctl, "%s", _("timeout is too big")); - return -1; - } - *timeout *= 1000; + + /* Ensure that we can multiply by 1000 without overflowing. */ + if (*timeout > INT_MAX / 1000) { + vshError(ctl, "%s", _("timeout is too big")); + return -1; } + *timeout *= 1000; return rv; } diff --git a/tools/virsh.h b/tools/virsh.h index 3e0251b..b217206 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -168,7 +168,7 @@ struct _vshCmdInfo { struct _vshCmdOptDef { const char *name; /* the name of option, or NULL for list end */ vshCmdOptType type; /* option type */ - unsigned int flags; /* flags */ + unsigned int flags; /* bitwise OR of VSH_OFLAG_**/ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ vshCompleter completer; /* option completer */ @@ -280,13 +280,14 @@ bool vshCmddefHelp(vshControl *ctl, const char *name); const vshCmdGrp *vshCmdGrpSearch(const char *grpname); bool vshCmdGrpHelp(vshControl *ctl, const char *name); -int vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) +int vshCommandOptInt(vshControl *ctl, const vshCmd *cmd, + const char *name, int *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -int vshCommandOptUInt(const vshCmd *cmd, const char *name, - unsigned int *value) +int vshCommandOptUInt(vshControl *ctl, const vshCmd *cmd, + const char *name, unsigned int *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -int vshCommandOptUL(const vshCmd *cmd, const char *name, - unsigned long *value) +int vshCommandOptUL(vshControl *ctl, const vshCmd *cmd, + const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; int vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) @@ -295,13 +296,14 @@ int vshCommandOptStringReq(vshControl *ctl, const vshCmd *cmd, const char *name, const char **value) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; -int vshCommandOptLongLong(const vshCmd *cmd, const char *name, - long long *value) +int vshCommandOptLongLong(vshControl *ctl, const vshCmd *cmd, + const char *name, long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -int vshCommandOptULongLong(const vshCmd *cmd, const char *name, - unsigned long long *value) +int vshCommandOptULongLong(vshControl *ctl, const vshCmd *cmd, + const char *name, unsigned long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -int vshCommandOptScaledInt(const vshCmd *cmd, const char *name, +int vshCommandOptScaledInt(vshControl *ctl, + const vshCmd *cmd, const char *name, unsigned long long *value, int scale, unsigned long long max) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; -- 1.9.0

2 1

[libvirt] [PATCH] build: Don't use code with dbus_message_unref when built without dbus
by Martin Kletzander 14 Apr '14

14 Apr '14

In order to do that, virNodeSuspendSupportsTargetPMUtils() and virSystemdPMSupportTarget() are created even when pm-utils and dbus are compiled out, respectively, but in that case returning -2 meaning "unavailable" (this return code was already used for unavailability before). Error is reported in virNodeSuspendSupportsTarget() only if both functions returned -2, otherwise the error (or success) is properly propagated up the stack. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- Notes: v1: https://www.redhat.com/archives/libvir-list/2014-April/msg00550.html This is a build-breaker if building --without-dbus due to dbus_message_unref being called unconditionally. However, I'm not pushing it as such. I haven't followed the suggestion of return codes -1, 0 and 1 because there is already a -2 used for unavailability and these return codes are used in other parts of the code for similar reasons as well (e.g. storage driver). src/util/virnodesuspend.c | 22 ++++++++++++++++++---- src/util/virsystemd.c | 11 ++++++++++- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 59b84ef..60b86ee 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -1,6 +1,7 @@ /* * virnodesuspend.c: Support for suspending a node (host machine) * + * Copyright (C) 2014 Red Hat, Inc. * Copyright (C) 2011 Srivatsa S. Bhat <srivatsa.bhat(a)linux.vnet.ibm.com> * * This library is free software; you can redistribute it and/or @@ -267,7 +268,14 @@ virNodeSuspendSupportsTargetPMUtils(unsigned int target, bool *supported) virCommandFree(cmd); return ret; } -#endif +#else /* ! WITH_PM_UTILS */ +static int +virNodeSuspendSupportsTargetPMUtils(unsigned int target ATTRIBUTE_UNUSED, + bool *supported ATTRIBUTE_UNUSED) +{ + return -2; +} +#endif /* ! WITH_PM_UTILS */ static int virNodeSuspendSupportsTargetSystemd(unsigned int target, bool *supported) @@ -313,10 +321,16 @@ virNodeSuspendSupportsTarget(unsigned int target, bool *supported) int ret; ret = virNodeSuspendSupportsTargetSystemd(target, supported); -#ifdef WITH_PM_UTILS - if (ret < 0) + + /* If just unavailable, try other options */ + if (ret == -2) ret = virNodeSuspendSupportsTargetPMUtils(target, supported); -#endif + + /* If still unavailable, then report error */ + if (ret == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot probe for supported suspend types")); + } return ret; } diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index e9ca564..e67956f 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -1,7 +1,7 @@ /* * virsystemd.c: helpers for using systemd APIs * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013, 2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -326,6 +326,7 @@ virSystemdNotifyStartup(void) #endif } +#ifdef WITH_SYSTEMD_DAEMON static int virSystemdPMSupportTarget(const char *methodName, bool *result) { @@ -369,6 +370,14 @@ virSystemdPMSupportTarget(const char *methodName, bool *result) return ret; } +#else /* ! WITH_SYSTEMD_DAEMON */ +static int +virSystemdPMSupportTarget(const char *methodName ATTRIBUTE_UNUSED, + bool *result ATTRIBUTE_UNUSED) +{ + return -2; +} +#endif /* ! WITH_SYSTEMD_DAEMON */ int virSystemdCanSuspend(bool *result) { -- 1.9.2

2 1

[libvirt] [PATCH] build: Embed dbus_message_unref in virdbus
by Martin Kletzander 14 Apr '14

14 Apr '14

Adding dbus_message_unref() into virDBusMessageDecodeArgs() makes sure that the message gets unref'd, thus making one more pure dbus call not necessary. Even though we are calling a lot of dbus_* functions outside virdbus (which should be fixed in the future IMHO), this patch fixes only this one instance because it merely aims to fix a build-breaker caused by improperly included dbus.h. The message printed when failing (using --without-dbus) is: util/virsystemd.c: In function 'virSystemdPMSupportTarget': util/virsystemd.c:367:5: error: implicit declaration of function 'dbus_message_unref' [-Werror=implicit-function-declaration] dbus_message_unref(message); ^ Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- Notes: I checked that we're not unreffing the message twice anywhere, but I wanted to check there is no crash if we do unref it twice. While trying whether double unreffing the message breaks anything, I saw that dbus_message_unref() has a refcount and it cleans structure when the refcount reaches 0, the cleansing means it sets it to -1 as well. OK, so it doesn't break anything, but the allocated structure is still not free()'d. I ran virsystemdtest under valgrind and saw that the message is really not free()'d. But here comes the best part; out of 6 test functions, only 3 of them are causing a leak and adding VIR_FREE(msg) to any test function other than the last one (which gets rid of one leak out of three) causes a segfault. I though dbus has it's own memory management or whatever, but looking at the backtrace, the crash happens when we are allocating DBusMessageIter *newiter one function later than the VIR_FREE(msg) is done (and msg is local to that function, btw). Even though this has nothing to do with the patch sent, I'd appreciate any explanation from anyone more dbus-knowledgeable than me (most probably anyone). src/util/virdbus.c | 1 + src/util/virsystemd.c | 3 +-- tests/virdbustest.c | 6 ------ 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 0cd3858..aef1d34 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1112,6 +1112,7 @@ int virDBusMessageDecodeArgs(DBusMessage* msg, } ret = virDBusMessageIterDecode(&iter, types, args); + dbus_message_unref(msg); cleanup: return ret; diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index e9ca564..9f67ac0 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -1,7 +1,7 @@ /* * virsystemd.c: helpers for using systemd APIs * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013, 2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -364,7 +364,6 @@ virSystemdPMSupportTarget(const char *methodName, bool *result) ret = 0; cleanup: - dbus_message_unref(message); VIR_FREE(response); return ret; diff --git a/tests/virdbustest.c b/tests/virdbustest.c index a798fbe..50578d9 100644 --- a/tests/virdbustest.c +++ b/tests/virdbustest.c @@ -121,7 +121,6 @@ static int testMessageSimple(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_string); VIR_FREE(out_signature); VIR_FREE(out_objectpath); - dbus_message_unref(msg); return ret; } @@ -171,7 +170,6 @@ static int testMessageVariant(const void *args ATTRIBUTE_UNUSED) cleanup: VIR_FREE(out_str1); VIR_FREE(out_str2); - dbus_message_unref(msg); return ret; } @@ -224,7 +222,6 @@ static int testMessageArray(const void *args ATTRIBUTE_UNUSED) cleanup: VIR_FREE(out_str1); VIR_FREE(out_str2); - dbus_message_unref(msg); return ret; } @@ -322,7 +319,6 @@ static int testMessageArrayRef(const void *args ATTRIBUTE_UNUSED) for (i = 0; i < out_nstrv2; i++) VIR_FREE(out_strv2[i]); VIR_FREE(out_strv2); - dbus_message_unref(msg); return ret; } @@ -397,7 +393,6 @@ static int testMessageStruct(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_string); VIR_FREE(out_signature); VIR_FREE(out_objectpath); - dbus_message_unref(msg); return ret; } @@ -467,7 +462,6 @@ static int testMessageDict(const void *args ATTRIBUTE_UNUSED) VIR_FREE(out_key1); VIR_FREE(out_key2); VIR_FREE(out_key3); - dbus_message_unref(msg); return ret; } -- 1.9.2

2 6

[libvirt] [PATCH] qemu: Avoid overflow when setting migration speed
by Jiri Denemark 14 Apr '14

14 Apr '14

When passing migration bandwidth to QEMU, we multiply it by 1024 * 1024 to convert the speed to B/s and the result still needs to fit in int64_t. https://bugzilla.redhat.com/show_bug.cgi?id=1083483 Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com> --- src/qemu/qemu_monitor.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5a5a59b..912bea1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2047,6 +2047,13 @@ int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon, return -1; } + if (bandwidth > QEMU_DOMAIN_MIG_BANDWIDTH_MAX) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + QEMU_DOMAIN_MIG_BANDWIDTH_MAX + 1ULL); + return -1; + } + if (mon->json) ret = qemuMonitorJSONSetMigrationSpeed(mon, bandwidth); else -- 1.9.2

2 2

[libvirt] [PATCH] RFC: bhyve: implement PCI address allocation
by Roman Bogorodskiy 14 Apr '14

14 Apr '14

Hi, This is the first attempt to implement PCI address allocation for the bhyve driver. This patch is by no means a complete version and the idea of this is to understand if I'm moving in a right direction. This code is based on the one from QEMU driver. Even though bhyve currently has no support for PCI bridges, it should be possible to add that support without major rewrites when this feature will be available in bhyve. So, currently we have the following. For a domain like that: https://gist.github.com/novel/10569989#file-domainin-xml The processed domain looks this way: https://gist.github.com/novel/10569989#file-domainout-xml and the command is: https://gist.github.com/novel/10569989#file-cmd Please let me know if it's uncomfortable to follow gist links; I didn't put domain xml inline as it's pretty lengthy. Open questions are: * What's the best way to deal with the 0:0,hostbridge device, should it be explicitly added to the domain definition? * How to handle lpc device, that's required for console. From bhyve point of view it looks like this: -s 31,lpc -l com1,ttydev That is, LPC PCI-ISA bridge on PCI slot and com1 port on that bridge. Things that need to be fixed in that patch, but are obvious: * Fix 'make check' which fails for obvious reasons of new addresses in domain xml * As we support more than one disk device, respect boot order Roman Bogorodskiy (1): bhyve: implement PCI address allocation po/POTFILES.in | 1 + src/Makefile.am | 4 + src/bhyve/bhyve_command.c | 128 +++++++++++----------- src/bhyve/bhyve_device.c | 272 ++++++++++++++++++++++++++++++++++++++++++++++ src/bhyve/bhyve_device.h | 41 +++++++ src/bhyve/bhyve_domain.c | 75 +++++++++++++ src/bhyve/bhyve_domain.h | 41 +++++++ src/bhyve/bhyve_driver.c | 9 +- 8 files changed, 503 insertions(+), 68 deletions(-) create mode 100644 src/bhyve/bhyve_device.c create mode 100644 src/bhyve/bhyve_device.h create mode 100644 src/bhyve/bhyve_domain.c create mode 100644 src/bhyve/bhyve_domain.h -- 1.9.0

2 3

[libvirt] [PATCH 0/2] bhyve: implement connectDomainXMLToNative
by Roman Bogorodskiy 14 Apr '14

14 Apr '14

One thing I'm wondering about. Bhyve has two step domain startup: 1. bhyveload 2. bhyve This implementation prints out only 'bhyve' command. Should it also print bhyveload command and if yes, how should it look for a user, something like 'bhyveload ... ; bhyve ...' ? Roman Bogorodskiy (2): bhyve: improve bhyve_command.c testability bhyve: implement connectDomainXMLToNative src/bhyve/bhyve_command.c | 85 +++++++++++++++++++++++++---------------------- src/bhyve/bhyve_command.h | 8 +++-- src/bhyve/bhyve_driver.c | 45 +++++++++++++++++++++++++ src/bhyve/bhyve_process.c | 10 +++--- tests/bhyvexml2argvtest.c | 2 +- 5 files changed, 103 insertions(+), 47 deletions(-) -- 1.9.0

2 4