Re: [libvirt] [PATCH] build: exclude more files from all the syntax checks
On Fri, Oct 6, 2017 at 7:47 AM, Eric Blake <ebl...@redhat.com> wrote: > On 10/05/2017 06:07 AM, Pino Toscano wrote: >> The majority of the syntax check is taylored for C sources, so some of >> the checks already cause false positives for non-C sources (and thus >> there are exclusion regexps in place). >> >> Instead, just exclude more non-C files from all the checks: >> - pot files: they are templates for po files (already excluded), and >> they are automatically generated from sources >> - pl files: Perl sources, which have own APIs, style, etc; they are >> helper scripts, not "real" sources >> - spec/spec.in files: RPM packaging files >> - js/woff/html.in files: files for web pages >> - diff/patch files: patches >> - stp files: SystemTap scripts >> - syms files: linker symbols files >> - conf files: generic configuration files >> - data/cpuinfo files: procinfo/cpuinfo files > > There are still some useful syntax checks for performing on ALL files > (for example, prohibit_doubled_word). So I'm not quite sure that > blindly exempting these files from all possible checks makes sense. > > Maybe it's worth teaching upstream gnulib syntax-check to make it easier > to auto-exclude non-C files from checks that ARE specific to the C > language, without weakening the global checks that are good on all > files. Maybe even something as simple as adding some sort of language= > tag to feed to $(_sc_search_regexp (if not specified, run on all files, > but if specified as C, the syntax-check is specific to C-like files, so > it limits to .h, .c. .y). > > I'm adding the gnulib list to get feedback on the idea; maybe Jim > Meyering has an opinion as one of the original syntax-check authors. Hi Eric, Is there some reason not to use a directive like this in a rule applying exclusively to version-controlled C-like files? in_vc_files='\.[chly]$$' I looked at libvirt's cfg.mk, and if you add that line to the sc_prohibit_sprintf rule, you can then remove the lines that exempt files with unrelated suffixes from that rule: exclude_file_name_regexp--sc_prohibit_sprintf = \ ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$ Another rule that can catch things in any non-binary file is sc_prohibit_undesirable_word_seq, even if it's only for pet peeves like detecting "can not". ... >> # Files that should never cause syntax check failures. >> VC_LIST_ALWAYS_EXCLUDE_REGEX = \ >> - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ >> + >> \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] filesystem recognition [was: [PATCH] util: recognize SMB filesystems as shared]
On Thu, Sep 26, 2013 at 7:33 AM, Eric Blake ebl...@redhat.com wrote: [actually adding coreutils] On 09/26/2013 08:28 AM, Eric Blake wrote: On 09/26/2013 03:43 AM, Laine Stump wrote: This should resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1012085 libvirt previously recognized NFS, GFS2, OCFS2, and AFS filesystems as shared, and thus eligible for exceptions to certain rules/actions about chowning image files before handing them off to a guest. This patch widens the definition of shared filesystem to include the SMB filesystem (sometimes called CIFS, or Windows file sharing). --- src/util/virstoragefile.c | 9 - src/util/virstoragefile.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) Coreutils includes a rather extensive list of file systems (alas, it's GPLv3+ code, so we can't use it verbatim without asking Jim Meyering and other coreutils folks to relax the license): http://git.sv.gnu.org/cgit/coreutils.git/tree/src/stat.c#n243 Would it be worth moving the list of known file systems, and knowledge of whether they are remote (shared) or local, out of coreutils and into a gnulib module, for reuse by other projects? Libvirt (LGPLv2+) has to make decisions based on what file system is hosting a guest image (guest migration behaves differently depending on whether the guest disk image resides on a local or a shared file system). Licensing may be a sticking point - coreutils' list is currently GPLv3+, but is derived from the kernel (GPLv2), and libvirt cannot reuse it unless it is further relaxed to LGPLv2+. Hi Eric, Moving it to gnulib sounds sensible, and I'm ok with the switch to LGPLv2+. It is currently v3 merely by virtue of residing in a .c file that calls exit. If you do this, please consider addressing this comment: case S_MAGIC_AUFS: /* 0x61756673 remote */ /* FIXME: change syntax or add an optional attribute like inotify:no. The above is labeled as remote so that tail always uses polling, but this isn't really a remote file system type. */ return aufs; It is referring to the fact that while initially local and remote were sufficient, for some file system types, the lines have blurred, making those terms misnomers. The intended boolean is something like has_inotify_support. Also, coreutils has makefile rules that try to add new types to the list -- typically we'd run those manually, just prior to a release. I'd like to preserve that functionality one way or another. Thanks, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] Fix QEMU save/restore with block devices
tags 6004 + notabug
close 6004
thanks
Eric Blake wrote:
[adding bug-coreutils]
On 04/22/2010 04:37 AM, Daniel P. Berrange wrote:
- if (virAsprintf(dest, exec:%s %s 2/dev/null, argstr,
safe_target) 0) {
+if (virAsprintf(dest, exec:%s | dd of=%s seek=%llub,
+argstr, safe_target, offset) 0) {
Don't you still need to silence stderr, particularly since dd writes to
stderr even on success? (2 instances)
I didn't want to silence stderr, because I want it to end up in the QEMU
logfile if anything goes wrong. So i really need a way to make dd keep
quiet on success, rather than throwing away stderr
Coreutils comes with an extension 'dd status=noxfer' which silences some
(but not all) output to stderr, but you'd have to test whether you are
targetting coreutils' dd (if dd comes from somewhere else, like busybox,
you'll cause a syntax error that prevents dd from doing anything at all).
There was a patch submitted to coreutils [1] that would add
status=noinfo, but it is currently held up by copyright status and lack
of documentation. Maybe I should revive that patch (or rather, write it
from scratch, to avoid copyright taint). But even so, you are still up
against the issue of testing whether you are targetting new-enough dd.
[1] http://lists.gnu.org/archive/html/bug-coreutils/2010-02/msg00161.html
...
As of coreutils-8.20,
dd now accepts 'status=none' to suppress all informational output.
so I've closed this.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] maint: ignore unsaved emacs files
Eric Blake wrote:
On 10/25/2012 05:12 PM, Laine Stump wrote:
What would be *really* nice is if git could give a *warning* if you
tried to do git add . and it found any of those files - it would prevent
the cases where you forget to save a file that you've modified.
If you write ChangeLog entries in advance, vc-dwim may do precisely
what you want:
http://www.gnu.org/s/vc-dwim/
in that it cross-checks that what you say you're going add, change, or
remove via ChangeLog with what the VC diffs say you're actually doing.
It also checks for editor temporaries, which usually indicate
your editor knows about a modified version that you have not
yet saved.
I suppose it IS possible to make 'git commit' complain loudly if you
have unsaved files, or if you tried to add a temporary file as part of
the commit, by modifying .git/hooks/pre-commit, but I'm not sure I have
the best formula for doing that off-hand, and it's the sort of thing
that has to be done in each copy of the tree (and thus, the sort of
thing you would want to automate during bootstrap).
CC'ing Jim, who has done just that in coreutils' bootstrap.conf, in
bootstrap_epilogue():
# Install our git hooks, as long as cp accepts the --backup option,
# so that we can back up any existing files.
case $(cp --help) in *--backup*) backup=1;; *) backup=0;; esac
if test $backup = 1; then
hooks=$(cd scripts/git-hooks git ls-files)
for f in $hooks; do
# If it is identical, skip it.
cmp scripts/git-hooks/$f .git/hooks/$f /dev/null \
continue
cp --backup=numbered scripts/git-hooks/$f .git/hooks
chmod a-w .git/hooks/$f
done
fi
Yep, that's how we install coreutils' custom log-checking
local commit hook script.
It sounds like you want a combination of that and the piece
of vc-dwim that looks for editor temporaries. It uses the names
of files that it's diffing or about to commit, and searches for
the each corresponding editor temporary file name (emacs and vi*).
If it finds a matching temporary file name, it complains.
Here's the Perl function from vc-dwim.pl:
# For emacs, the temporary is a symlink named $dir/.#$base,
# with useful information in the link name part.
# For Vim, the temporary is a regular file named $dir/.$base.swp.
# Vim temporaries can also be named .$base.swo, .$base.swn, .$base.swm, etc.
# so test for a few of those, in the unusual event that one of those
# exists, but the .swp file does not.
sub exists_editor_backup ($)
{
my ($f) = @_;
# If $f is a symlink, use its referent.
-l $f
and $f = readlink $f;
my $d = dirname $f;
$f = basename $f;
my @candidate_tmp =
(
$d/.#$f, $d/#$f#, # Emacs
map { $d/.$f.sw$_ } qw (p o n m l k),# Vim
);
foreach my $c (@candidate_tmp)
{
-l $c or -f _
and return $c; # Vim
}
return undef;
}
(For that matter, it would be nice if make did that :-)
I run make all the time with unsaved files; in that scenario, it would
have to be only a warning and not an error. I'm more comfortable with
it being fatal only in a git hook (which happens when you try to commit
a mess) and not during development (where a mess might be normal, as
part of testing a half-baked theory). But indeed, if there were a way
to make maint.mk (or cfg.mk) install some GNU make constructs to add a
loud warning on the presence of any editor temporary files, as evidence
that we have sniffed an unsaved file so the build may fail in relation
to what you have just typed into your editor, that might be a nice
improvement.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] libvirt build failure w/GNU make and automake.git (automake regression?)
When I run ./autogen.sh make, I see this: (this arose because I had the latest automake.git/master tools -- commit c1b83e1af60b866cf5cdeebf77d0275019bad8b2 from today -- early in my path) Generated 3 wrapper functions CC libvirtmod_la-libvirt-override.lo CC libvirtmod_la-typewrappers.lo CC libvirtmod_la-libvirt.lo CC libvirtmod_qemu_la-libvirt-qemu-override.lo CC libvirtmod_qemu_la-typewrappers.lo CC libvirtmod_qemu_la-libvirt-qemu.lo CCLD libvirtmod_qemu.la CCLD libvirtmod.la make[3]: Leaving directory `/h/j/w/co/libvirt/python' Making all in tests make[3]: Entering directory `/h/j/w/co/libvirt/python/tests' make[3]: Nothing to be done for `all'. make[3]: Leaving directory `/h/j/w/co/libvirt/python/tests' make[2]: Leaving directory `/h/j/w/co/libvirt/python' Making all in tests make[2]: Entering directory `/h/j/w/co/libvirt/tests' Makefile:4355: *** Malformed target-specific variable definition. Stop. make[2]: Leaving directory `/h/j/w/co/libvirt/tests' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/h/j/w/co/libvirt' make: *** [all] Error 2 That is because of this automake-generated rule: undefine.log: undefine @p='undefine'; \ b='undefine'; \ $(am__check_pre) $(LOG_DRIVER) --test-name $$f \ --log-file $$b.log --trs-file $$b.trs \ $(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \ $$tst $(AM_TESTS_FD_REDIRECT) The trouble is that undefine is an operator in GNU make. Here's that part of GNU make's documentation: 6.9 Undefining Variables If you want to clear a variable, setting its value to empty is usually sufficient. Expanding such a variable will yield the same result (empty string) regardless of whether it was set or not. However, if you are using the `flavor' (*note Flavor Function::) and `origin' (*note Origin Function::) functions, there is a difference between a variable that was never set and a variable with an empty value. In such situations you may want to use the `undefine' directive to make a variable appear as if it was never set. For example: foo := foo bar = bar undefine foo undefine bar $(info $(origin foo)) $(info $(flavor bar)) This example will print undefined for both variables. If you want to undefine a command-line variable definition, you can use the `override' directive together with `undefine', similar to how this is done for variable definitions: override undefine CFLAGS The most pragmatic work-around is to rename the undefine test script. However, Stephano, as automake maintainer, I think you will want to fix automake not to prohibit the use of such test names. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] 100+ misspellings
Hello, In case someone is interested in weeding out the inevitable false positives, here is a slightly filtered list of misspelled words in libvirt, as detected by the misspellings program from here: http://github.com/lyda/misspell-check Here's the output from this command: misspellings $(git ls-files)|grep -vE 'gnulib/|po/|ChangeLog-old' HACKING:456: particulary - particularly configure.ac:1359: presense - presence daemon/libvirtd.conf:254: upto - up to daemon/libvirtd.conf:270: upto - up to daemon/stream.c:692: upto - up to docs/api.html.in:67: garanteed - guaranteed docs/api.html.in:73: garanteed - guaranteed docs/apibuild.py:2476: unkown - unknown docs/apps.html.in:191: upto - up to docs/firewall.html.in:320: managable - manageable,manageably docs/formatdomain.html.in:423: ommitted - omitted docs/hacking.html.in:547: particulary - particularly docs/internals/locking.html.in:153: psuedo - pseudo docs/news.html.in:610: commited - committed docs/news.html.in:1195: dependancy - dependency docs/news.html.in:1459: doesnt - doesn't docs/news.html.in:1570: dependancy - dependency docs/news.html.in:1837: stange - strange docs/news.html.in:1838: dependancy - dependency docs/news.html.in:1892: sucessful - successful docs/news.html.in:1893: sucessful - successful docs/news.html.in:2032: sucessful - successful docs/news.html.in:2472: orginal - original docs/news.html.in:2628: statment - statement docs/news.html.in:2793: controling - controlling docs/news.html.in:3912: Seperate - Separate docs/news.html.in:5025: occurence - occurrence docs/news.html.in:5592: Explictly - Explicitly docs/news.html.in:6508: beeing - being docs/news.html.in:7158: adressing - addressing,dressing docs/news.html.in:7483: Supress - Suppress docs/news.html.in:7542: occurence - occurrence docs/news.html.in:7551: sucess - success docs/news.html.in:7825: neccessary - necessary docs/news.html.in:7902: Occurence - Occurrence docs/news.html.in:7902: occurences - occurrences docs/news.html.in:8472: wierd - weird docs/news.html.in:8535: accomodate - accommodate docs/news.html.in:8598: dependancy - dependency docs/news.html.in:9008: dependancy - dependency docs/news.html.in:9306: dependancy - dependency docs/news.html.in:9324: dependance - dependence docs/news.html.in:9520: extention - extension docs/news.html.in:9587: dependance - dependence docs/remote.html.in:359: agains - against docs/schemas/interface.rng:7: everytime - every time docs/schemas/interface.rng:175: grat - great examples/domain-events/events-python/event-test.py:129: occurr - occur examples/domain-events/events-python/event-test.py:172: upto - up to examples/python/dominfo.py:46: runing - running include/libvirt/libvirt.h.in:642: targetting - targeting include/libvirt/libvirt.h.in:2935: specfic - specific include/libvirt/libvirt.h.in:3207: upto - up to include/libvirt/libvirt.h.in:3639: occuring - occurring src/conf/domain_conf.c:4565: Unkown - Unknown src/conf/domain_conf.c:4933: Unkown - Unknown src/conf/domain_conf.c:6914: Wierd - Weird src/conf/nwfilter_conf.c:2804: definiton - definition src/internal.h:114: conciously - consciously src/libvirt.c:3083: transfered - transferred src/libvirt.c:4390: dependant - dependent src/libvirt.c:4441: dependant - dependent src/libvirt.c:11461: upto - up to src/libvirt.c:11544: upto - up to src/libvirt.c:14119: funtion - function src/libvirt.c:14542: dependant - dependent src/libvirt_internal.h:2: publically - publicly src/phyp/phyp_driver.c:2067: avaliable - available src/phyp/phyp_driver.c:2290: avaliable - available src/phyp/phyp_driver.c:2770: avaliable - available src/qemu/qemu_capabilities.c:659: existance - existence src/qemu/qemu_capabilities.c:1234: targetting - targeting src/qemu/qemu_conf.c:205: lenght - length src/qemu/qemu_driver.c:1235: calulate - calculate src/qemu/qemu_monitor_text.c:2406: everthing - everything src/qemu/qemu_process.c:2657: pased - passed src/rpc/virnetsaslcontext.c:153: Succesful - Successful src/rpc/virnettlscontext.c:377: Succesful - Successful src/storage/storage_backend_disk.c:536: boundry - boundary src/storage/storage_backend_disk.c:561: boundry - boundary src/storage/storage_backend_disk.c:571: boundry - boundary src/storage/storage_backend_disk.c:620: boundry - boundary src/storage/storage_backend_rbd.c:318: occured - occurred src/uml/uml_driver.c:260: upto - up to src/uml/uml_driver.c:1293: calulate - calculate src/util/cgroup.c:69: HIERACHY - Hierarchy src/util/cgroup.c:172: neccessarily - necessarily src/util/cgroup.c:592: HIERACHY - Hierarchy src/util/cgroup.c:1007: HIERACHY - Hierarchy src/util/cgroup.c:2029: existance - existence src/util/processinfo.c:80: upto - up to src/util/processinfo.c:145: upto - up to src/util/virpidfile.c:369: agin - again src/util/virpidfile.c:378: agin - again src/util/virsocketaddr.c:421: recieve - receive src/util/virsocketaddr.c:451: recieve - receive src/util/virtime.c:105: THRU - Through src/util/virtime.c:162: THRU - Through src/util/virtime.c:163: THRU -
Re: [libvirt] [PATCH] maint: don't permit format strings without %
Eric Blake wrote:
Any time we have a string with no % passed through gettext, a
translator can inject a % to cause a stack overread. When there
is nothing to format, it's easier to ask for a string that cannot
be used as a formatter, by using a trivial %s format instead.
In the past, we have used --disable-nls to catch some of the
offenders, but that doesn't get run very often, and many more
uses have crept in. Syntax check to the rescue!
The syntax check can catch uses such as
virReportError(code,
_(split
string));
by using a sed script to fold context lines into one pattern
space before checking for a string without %.
This patch is just mechanical insertion of %s; there are probably
several messages touched by this patch where we would be better
off giving the user more information than a fixed string.
...
diff --git a/cfg.mk b/cfg.mk
index d054e5a..085ef34 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -547,7 +547,7 @@ msg_gen_function += xenapiSessionErrorHandler
# msg_gen_function += vshPrint
# msg_gen_function += vshError
-func_or := $(shell printf '$(msg_gen_function)'|tr -s '[[:space:]]' '|')
+func_or := $(shell echo $(msg_gen_function)|tr -s '[[:space:]]' '|')
func_re := ($(func_or))
# Look for diagnostics that aren't marked for translation.
@@ -567,6 +567,19 @@ sc_libvirt_unmarked_diagnostics:
{ echo '$(ME): found unmarked diagnostic(s)' 12;\
exit 1; } || :
+# Look for diagnostics that lack a % in the format string, except that we
+# allow VIR_ERROR to do this, and ignore functions that take a single
+# string rather than a format argument.
+sc_prohibit_diagnostic_without_format:
+ @{ grep -nE '\$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
+grep -A2 -nE '\$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \
+| sed -rn -e ':l; /[,]$$/ {N;b l;}' \
+ -e '/xenapiSessionErrorHandler/d'\
+ -e '/\$(func_re) *\([^]*([^%]|\n[^]*)*[,)]/p'\
+ | grep -vE '(VIR_ERROR|vah_(error|warning))' \
+ { echo '$(ME): found diagnostic without %' 12; \
+ exit 1; } || :
+
# Like the above, but prohibit a newline at the end of a diagnostic.
# This is subject to false positives partly because it naively looks for
# `\n', which may not be the end of the string, and also because it takes
Hi Eric,
Nice patch.
The subject piqued my interest, so even on PTO, I took a quick look.
One issue (I haven't gone through the whole thing)
is that you should not insert the new rule there, because
by separating the preceding and following rules, it would
render invalid the following Like the above... comment.
Also, I didn't see right off why you've changed from using
printf to using echo in the definition of func_or.
Without looking at the definition of msg_gen_function, I
suspect that switching to echo will add an unwanted trailing |, no?
Seeing no ChangeLog entry, I wondered if it was an accident.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: reduce length of nodeinfodata test names
Eric Blake wrote: ... This [change-set] LOOKS huge, but is really just a couple of 'git mv' commands and a change to nodeinfotest.c. Scroll to the bottom for the real change; I've shortened boring parts of this mail to get past the 100k mail cap enforced by the list. If you set git diff's renames option, (i.e., globally): git config --global diff.renames true then git diff, format-patch, etc. output is much more readable, i.e., it excludes the remove-entire-file then add-same-contents diff output. Instead it just prints something like this: diff --git a/old b/new similarity index 100% rename from old rename to new -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] maint: avoid new automake warning about AM_PROG_CC_STDC
When I built libvirt recently, I saw a warning go by: (with cutting edge automake-1.12a) configure.ac:83: warning: macro 'AM_PROG_CC_STDC' not found in library This fixes it: From 8f66a7e7f12efa2a60754f06627e94d96030ac78 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Sat, 26 May 2012 11:21:47 +0200 Subject: [PATCH] maint: avoid new automake warning about AM_PROG_CC_STDC * configure.ac (AM_PROG_CC_STDC): Stop using this macro. It provokes warnings from newer automake and is superseded by autoconf's AC_PROG_CC, which we're already using. --- configure.ac | 1 - 1 file changed, 1 deletion(-) diff --git a/configure.ac b/configure.ac index 5d06b47..54f51ee 100644 --- a/configure.ac +++ b/configure.ac @@ -80,7 +80,6 @@ dnl Checks for C compiler. AC_PROG_CC AC_PROG_INSTALL AC_PROG_CPP -AM_PROG_CC_STDC gl_EARLY gl_INIT -- 1.7.10.2.565.gbd578b5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] maint: avoid new automake warning about AM_PROG_CC_STDC
Eric Blake wrote: On 05/26/2012 03:25 AM, Jim Meyering wrote: When I built libvirt recently, I saw a warning go by: (with cutting edge automake-1.12a) configure.ac:83: warning: macro 'AM_PROG_CC_STDC' not found in library This fixes it: From 8f66a7e7f12efa2a60754f06627e94d96030ac78 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Sat, 26 May 2012 11:21:47 +0200 Subject: [PATCH] maint: avoid new automake warning about AM_PROG_CC_STDC * configure.ac (AM_PROG_CC_STDC): Stop using this macro. It provokes warnings from newer automake and is superseded by autoconf's AC_PROG_CC, which we're already using. --- configure.ac | 1 - 1 file changed, 1 deletion(-) ACK. Thanks. Pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virsh: avoid heap corruption leading to virsh abort
Investigating a build problem reported by Laine,
I was surprised to see make check fail on F17 due to a
glibc invalid free abort. Ok to push to master?
From 61a559e0b2f4bded3059c5be7c958e2276f7fd16 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Mon, 7 May 2012 21:22:09 +0200
Subject: [PATCH] virsh: avoid heap corruption leading to virsh abort
* tools/virsh.c (vshParseSnapshotDiskspec): Fix off-by-3 memmove
that would corrupt heap when parsing escaped --diskspec comma.
Bug introduced via commit v0.9.4-260-g35d52b5.
---
tools/virsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 1207ac9..dd9292a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -16107,7 +16107,7 @@ vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr
buf, const char *str)
while ((tmp = strchr(tmp, ','))) {
if (tmp[1] == ',') {
/* Recognize ,, as an escape for a literal comma */
-memmove(tmp[1], tmp[2], len - (tmp - spec) + 2);
+memmove(tmp[1], tmp[2], len - (tmp - spec) - 2 + 1);
len--;
tmp++;
continue;
--
1.7.10.1.457.g8275905
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: avoid heap corruption leading to virsh abort
Eric Blake wrote:
On 05/07/2012 01:29 PM, Jim Meyering wrote:
Investigating a build problem reported by Laine,
I was surprised to see make check fail on F17 due to a
glibc invalid free abort. Ok to push to master?
From 61a559e0b2f4bded3059c5be7c958e2276f7fd16 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Mon, 7 May 2012 21:22:09 +0200
Subject: [PATCH] virsh: avoid heap corruption leading to virsh abort
* tools/virsh.c (vshParseSnapshotDiskspec): Fix off-by-3 memmove
that would corrupt heap when parsing escaped --diskspec comma.
Bug introduced via commit v0.9.4-260-g35d52b5.
---
tools/virsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 1207ac9..dd9292a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -16107,7 +16107,7 @@ vshParseSnapshotDiskspec(vshControl *ctl,
virBufferPtr buf, const char *str)
while ((tmp = strchr(tmp, ','))) {
if (tmp[1] == ',') {
/* Recognize ,, as an escape for a literal comma */
-memmove(tmp[1], tmp[2], len - (tmp - spec) + 2);
+memmove(tmp[1], tmp[2], len - (tmp - spec) - 2 + 1);
ACK.
Thanks for the quick review. Pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] build: Fix version of gettext macros
Eric Blake wrote: [adding bug-gnulib] On 04/24/2012 06:22 AM, Eric Blake wrote: On 04/24/2012 03:50 AM, Peter Krempa wrote: Commit c9cd419caba9effa11ca53e8696e5f6a4b424d60 added copying of the makefile for translation files from gnulib. The makefile from gnulib is of version 0.18 but the build configuration cretes macros from version 0.17 which breaks the build with message: *** error: gettext infrastructure mismatch: using a Makefile.in.in from gettext version 0.18 but the autoconf macros are from gettext version 0.17 --- bootstrap.conf |2 +- configure.ac |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) NACK. RHEL 5 still uses gettext 0.17, and this breaks the build there. Not just RHEL 5, but RHEL 6.2 as well. We need to fix gnulib to not force us to use gettext 0.18. I'll look into this. Here's what I'm playing with now; so far, it appears to make life happy for libvirt with its intentional AM_GNU_GETTEXT_VERSION([0.17]). Jim, does this look like a reasonable approach? Any suggestions before we make it official in gnulib? Hi Eric, That change is effectively disabling the build-time check that ensures Makefile.in.in and gettext.am versions are in sync. That version comparison is definitely the problem[*], but I haven't tried using an older gettext.m4 with newer Makefile.in.in like you propose to do. I have just inspected gettext's v0.17..HEAD Makefile.in.in diffs, and don't see a problem in this particular case. I think libvirt builds will work fine with those two files mismatched. However, I remember (too well: http://bugzilla.redhat.com/523713) that using some older-still version of gettext.m4 caused a hard-to-diagnose failure when used with newer Makefile.in.in. Since it could cause trouble with other (and perhaps future) combinations of version numbers, any such disabling should remain libvirt-specific. Possible alternative: a very specific transformation that would disable the test only in the precise 0.17-vs-0.18 case, once you have confirmed it is ok. An alternative that I've always advocated, when developing on systems with build tools that are out of date, is to build and install private copies of the latest tools. This script automates that task: http://people.redhat.com/meyering/autotools-install See its --help output for a recommended way to invoke it. [*] Rather than comparing gettext version numbers, it could be comparing some version number associated with the API that covers those two files. I suppose that this API version number would change less frequently than the gettext package version number. diff --git i/bootstrap w/bootstrap index 5aa73cc..1cacd03 100755 --- i/bootstrap +++ w/bootstrap @@ -1,6 +1,6 @@ #! /bin/sh # Print a version string. -scriptversion=2012-04-19.22; # UTC +scriptversion=2012-04-25.04; # UTC # Bootstrap this package from checked-out sources. @@ -873,7 +873,14 @@ if test $with_gettext = yes; then } ' po/Makevars.template po/Makevars || exit 1 - cat $GNULIB_SRCDIR/build-aux/po/Makefile.in.in po/Makefile.in.in || exit 1 + gettext_sed='s/^[ ]*AM_GNU_GETTEXT_VERSION(\[*\([^]]*\)\]*).*/\1/p' + gettext_version=$(sed -n $gettext_sed configure.ac) + if test -n $gettext_version; then +sed s/\(Origin:.*-\|MACRO_VERSION = \)0\.[0-9][0-9]/\1$gettext_version/ \ + $GNULIB_SRCDIR/build-aux/po/Makefile.in.in po/Makefile.in.in || exit 1 + else +cp $GNULIB_SRCDIR/build-aux/po/Makefile.in.in po/Makefile.in.in || exit 1 + fi if test -d runtime-po; then # Similarly for runtime-po/Makevars, but not quite the same. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: remove unneeded #include in virrandom.c
Eric Blake wrote: On 03/01/2012 09:53 AM, Daniel P. Berrange wrote: On Thu, Mar 01, 2012 at 11:43:04AM -0500, Laine Stump wrote: Commit 7c90026 added #include conf/domain_conf.h to util/virrandom.c. Fortunately it didn't actually use anything from domain_conf.h, since as far as I'm aware, files in util aren't allowed to reference anything in conf (although the opposite is allowed). So this #include is unnecessary. That is correct. util/ must be self-contained Guess what - that sounds like a great syntax rule to write, so we don't slip up in the future. Give me a few minutes, to see what I can come up with. Any other directories that should be avoiding particular includes? I noticed Laine's message, admired his control and wrote this, came back to reply and found your message. Laine, you're welcome to merge this into your commit. diff --git a/cfg.mk b/cfg.mk index ac6c527..ca6fe65 100644 --- a/cfg.mk +++ b/cfg.mk @@ -624,6 +624,13 @@ sc_prohibit_gettext_markup: halt='do not mark these strings for translation'\ $(_sc_search_regexp) +# One must not include conf/ headers from src/util/. +sc_prohibit_conf_inclusion_from_util: + @in_vc_files='^src/util/' \ + prohibit='^# *include conf/' \ + halt='do not include conf/*.h from src/util/*' \ + $(_sc_search_regexp) + # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker: -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] FYI, Unable to create cgroup for ...
Daniel P. Berrange wrote: On Tue, Mar 29, 2011 at 02:55:28PM +0200, Jim Meyering wrote: In case someone else encounters this... Today none of my VMs would start on F15: # virsh create rawhide.xml error: Failed to create domain from rawhide.xml error: Unable to create cgroup for rawhide: No such file or directory Luckily, I noticed that Rich Jones had the same problem a week ago and he found that restarting libvirtd was enough to solve the problem: # service libvirtd restart That did it for me, too. This is a bug in systemd. It periodically scans all mounted cgroups and deletes any directories which don't contain any attached processes. Needless to say this breaks libvirt, and possibly other apps, which don't expect 3rd parties to be deleting their directories. https://bugzilla.redhat.com/show_bug.cgi?id=678555 Thanks. With that, it's sure to be fixed soon. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] FYI, Unable to create cgroup for ...
In case someone else encounters this... Today none of my VMs would start on F15: # virsh create rawhide.xml error: Failed to create domain from rawhide.xml error: Unable to create cgroup for rawhide: No such file or directory Luckily, I noticed that Rich Jones had the same problem a week ago and he found that restarting libvirtd was enough to solve the problem: # service libvirtd restart That did it for me, too. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] don't check for NULL before calling virHashFree
Eric Blake wrote:
[adding Jim to cc, as author of useless-if-before-free]
On 02/17/2011 02:14 PM, Christophe Fergeau wrote:
virHashFree follows the convention described in HACKING that
XXXFree() functions can be called with a NULL argument.
---
src/conf/domain_conf.c |6 +---
src/datatypes.c | 51
+++---
src/qemu/qemu_process.c |4 +--
3 files changed, 20 insertions(+), 41 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 351daf7..e7c3409 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -410,8 +410,7 @@ static void virDomainObjListDeallocator(void
*payload, const char *name ATTRIBUT
void virDomainObjListDeinit(virDomainObjListPtr doms)
{
-if (doms-objs)
-virHashFree(doms-objs, virDomainObjListDeallocator);
+virHashFree(doms-objs, virDomainObjListDeallocator);
I tried adding --name=virHashFree to the useless_free_options variable
in cfg.mk, to see if that would prevent regressions. However, it
appears that this two-argument free-like function is not picked up by
the heuristics in the useless-if-before-free script (it only works on
one-argument functions).
ACK to your patch as-is, so I pushed it. But I can't help but wonder if
we could make this easier to enforce at 'make syntax-check' time by
tweaking the perl script somehow.
Right. useless-if-before-free deliberately detects only
one-argument free-like functions.
I'm reluctant to add such a change to that script because
- there are comparatively few free-like functions with more than
one argument, so there's not much anticipated gain
- we don't know in general whether the free'd pointer arg is the
first or second, so...
- having to match against either the first or 2nd argument would
greatly complicate the already complicated regular expressions
If you're interested in pursuing it, I suggest switching tools
altogether and using spatch. That would make for a much cleaner
(albeit probably less efficient) script. It would also depend
on a less-ubiquitous tool than perl.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: replace CRLF with LF
Eric Blake wrote: On 01/28/2011 05:52 AM, Juerg Haefliger wrote: --- docs/page.xsl | 350 1 files changed, 175 insertions(+), 175 deletions(-) ACK, applied. po/ca.po is the only other file with bad line endings, but that requires some coordination with the translation site to get fixed permanently. I used this to look: $ git grep -l $'\r' | grep -Ev '\.(png|gif|ico)$' It may be worth enabling the (relatively recent git addition of) core.whitespace.cr-at-eol option to enforce no CR in text files (as well as using .gitattributes to mark .png/gif/ico as binary files that are exempt), but I'm not sure if that is something that we can enforce as a server side hook (so I'm cc'ing Jim for advice). I don't know off hand, but if possible, it sure looks like it'd be worth setting up. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] compressing .git/ can save 15%
Hi DV, I ran this on a fresh clone of libvirt: $ du -sh .git; git gc --aggressive; du -sh .git 54M .git ... 45M .git I propose to do the same thing on the server, libvirt.org. It's not a big deal, but decreased bandwidth wouldn't hurt, and the slightly smaller on-disk repository makes even local git tools feel a little snappier. It's worth doing for all git repositories. As far as I know, no one stores anything useful as unlinked commits on the server (they would be removed by the above), so there is no down-side to doing this. In fact, man git-gc recommends to run git gc --aggressive every few hundred changesets or so. Adding a cron job to do it every few weeks would be nice (e.g., Sunday at 4am local). Let me know and I'll do it via my account. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] compressing .git/ can save 15%
Daniel Veillard wrote: ... xmlsoft:~ - cat bin/GitMaintainance #!/bin/sh # Garbage collect the libvirt GIT repository git --git-dir /git/libvirt.git gc $HOME/tmp/GitMaintainance.log 21 xmlsoft:~ - crontab -l | grep GitMaintainance 4 4 * * Sun /u/veillard/bin/GitMaintainance xmlsoft:~ - Seems to have run normally on Sun 9 Jan from what I can tell :-) So basically I should just add --aggressive to the garbage collection command for extra cleanup, right ? Exactly ;-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] compressing .git/ can save 15%
Daniel Veillard wrote: On Wed, Jan 12, 2011 at 12:01:12PM +0100, Jim Meyering wrote: Daniel Veillard wrote: ... xmlsoft:~ - cat bin/GitMaintainance #!/bin/sh # Garbage collect the libvirt GIT repository git --git-dir /git/libvirt.git gc $HOME/tmp/GitMaintainance.log 21 xmlsoft:~ - crontab -l | grep GitMaintainance 4 4 * * Sun /u/veillard/bin/GitMaintainance xmlsoft:~ - Seems to have run normally on Sun 9 Jan from what I can tell :-) So basically I should just add --aggressive to the garbage collection command for extra cleanup, right ? Exactly ;-) Okay, done, BTW here is the result for a manual run I did: xmlsoft:~ - git --git-dir /git/libvirt.git gc --aggressive Counting objects: 50594, done. Delta compression using up to 2 threads. Compressing objects: 100% (50455/50455), done. Writing objects: 100% (50594/50594), done. Total 50594 (delta 42525), reused 0 (delta 0) xmlsoft:~ - That tells you the number of objects, which doesn't change. du -sh /git/libvirt.git before showed 54M. Now, it prints 45M. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Bootstrap fail, but non-useful message :/
Justin Clift wrote: On 30/11/2010, at 12:35 AM, Matthias Bolte wrote: Looks like the system is missing pkg-config, or at least has the .m4 file is in an unexpected place so that autoconf can't find it. bootstrap checks for pkg-config, so the binary is probably there, but that doesn't mean that autoconf can find the corresponding .m4 file, if it's there at all. Interesting. When installing autoconf, MacPorts also installs a newer m4 than the system provided one too. I wonder if there's a better way to cope with this m4 related failure scenario, than the text presently being given? I'm sure there is, but is it worth it? If lots of developers hit this, then maybe... For now, your best bet might be to build the latest versions of those tools yourself, e.g., via this script: http://people.redhat.com/meyering/autotools-install Once you've downloaded it, run bash autotools-install --help -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] show compiled in options in virsh --version
Eric Blake wrote: [adding bug-gnulib] On 10/28/2010 01:25 PM, Daniel Veillard wrote: case 'v': -/* FIXME - list a copyright blurb, as in GNU programs? */ -puts(VERSION); +vshShowVersion(ctl); exit(EXIT_SUCCESS); Pre-existing bug - we don't detect write failure to stdout to exit with non-zero status. Unchanged by your patch. Not sure it's really a problem, but okay I will look. The gnulib module closeout can automatically take care of this, but it is currently licensed as GPL. Jim, Bruno, are you okay with relicensing closeout and close-stream as LGPLv2+? They're both relatively thin wrappers. Fine by me. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] show compiled in options in virsh --version
Bruno Haible wrote: Hi Eric, case 'v': -/* FIXME - list a copyright blurb, as in GNU programs? */ -puts(VERSION); +vshShowVersion(ctl); exit(EXIT_SUCCESS); The gnulib module closeout can automatically take care of this, but it is currently licensed as GPL. Jim, Bruno, are you okay with relicensing closeout and close-stream as LGPLv2+? In gnulib we have a rule of thumb which says that anything that calls exit() or xmalloc() is likely to end up only in executables, not in libraries, and code in executables can just as well be relicensed under GPL instead of LGPL. Of course. Thanks for the reminder, Bruno. That is indeed a slippery slope. I was too hasty in saying ok. Eric, one option is to use the GPLv3 for programs like virsh, then to have a second gnulib library that they would use. In the libiconv and gettext packages, for example, the executables are all under GPL, although other parts of the package are under LGPL. Have you (and the management and lawyers behind libvirt) considered this approach for libvirt? I ask because 'closeout' may be only the beginning. Then comes 'xalloc', 'quotearg', and 'avltree-list', and at the end these high-value modules could be used by proprietary programs (assuming appropriate LGPL compliant packaging). We need a borderline between what can go LGPL and what needs to stay GPL, and the rule of thumb mentioned above is a good guiding line. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuConnectMonitor: do not mask SELinux failure
Matthias Bolte wrote: 2010/7/13 Jim Meyering j...@meyering.net: coverity spotted the expressions that could never be true: (a b c) 0. Here's the fix: From 6d61d90a6df81900f4b2be752403758175000973 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Tue, 13 Jul 2010 15:15:04 -0500 Subject: [PATCH] qemuConnectMonitor: fix a bug that would have masked SELinux failure * src/qemu/qemu_driver.c (qemuConnectMonitor): Correct erroneous parenthesization in two expressions. Without this fix, failure to set or clear SELinux security context in the monitor would go undiagnosed. Also correct a diagnostic and split some long lines. --- ACK. Hi Matthias, Thanks for the review. I've pushed that. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] uml_driver: correct logic error in umlMonitorCommand
Matthias Bolte wrote:
2010/7/13 Jim Meyering j...@meyering.net:
The stray 0 removed by the fix below
would have made umlMonitorCommand always fail
with its incomplete reply ... diagnostic.
From 5f075332e33f7e9bf2d7979934b8dc7c3312b6e8 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 13 Jul 2010 15:28:35 -0500
Subject: [PATCH] uml_driver: correct logic error in umlMonitorCommand
* src/uml/uml_driver.c (umlMonitorCommand): Correct flaw that would
cause unconditional incomplete reply ... failure, since nbytes
was always 0 or 1.
---
src/uml/uml_driver.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 110179e..1e0f5ac 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -730,7 +730,7 @@ static int umlMonitorCommand(const struct uml_driver
*driver,
ssize_t nbytes;
addrlen = sizeof(addr);
nbytes = recvfrom(priv-monitor, res, sizeof res, 0,
- (struct sockaddr *)addr, addrlen) 0;
+ (struct sockaddr *)addr, addrlen);
if (nbytes 0) {
if (errno == EAGAIN || errno == EINTR)
continue;
--
1.7.1.460.gf3c4c
I wondered how the 0 got there and git blame told me it's a leftover
from refactoring recvfrom out of 'if (nbytes 0)'.
ACK.
Thanks! I've pushed that, too.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemuConnectMonitor: do not mask SELinux failure
coverity spotted the expressions that could never be true:
(a b c) 0. Here's the fix:
From 6d61d90a6df81900f4b2be752403758175000973 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 13 Jul 2010 15:15:04 -0500
Subject: [PATCH] qemuConnectMonitor: fix a bug that would have masked SELinux
failure
* src/qemu/qemu_driver.c (qemuConnectMonitor): Correct erroneous
parenthesization in two expressions. Without this fix, failure
to set or clear SELinux security context in the monitor would go
undiagnosed. Also correct a diagnostic and split some long lines.
---
src/qemu/qemu_driver.c | 20
1 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 487bfa3..96277cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1194,10 +1194,12 @@ qemuConnectMonitor(struct qemud_driver *driver,
virDomainObjPtr vm)
qemuDomainObjPrivatePtr priv = vm-privateData;
int ret = -1;
-if ((driver-securityDriver
- driver-securityDriver-domainSetSecuritySocketLabel
-
driver-securityDriver-domainSetSecuritySocketLabel(driver-securityDriver,vm))
0) {
-VIR_ERROR(_(Failed to set security context for monitor for %s),
vm-def-name);
+if (driver-securityDriver
+driver-securityDriver-domainSetSecuritySocketLabel
+driver-securityDriver-domainSetSecuritySocketLabel
+ (driver-securityDriver,vm) 0) {
+VIR_ERROR(_(Failed to set security context for monitor for %s),
+ vm-def-name);
goto error;
}
@@ -1213,10 +1215,12 @@ qemuConnectMonitor(struct qemud_driver *driver,
virDomainObjPtr vm)
if (priv-mon == NULL)
virDomainObjUnref(vm);
-if ((driver-securityDriver
- driver-securityDriver-domainClearSecuritySocketLabel
-
driver-securityDriver-domainClearSecuritySocketLabel(driver-securityDriver,vm))
0) {
-VIR_ERROR(_(Failed to set security context for monitor for %s),
vm-def-name);
+if (driver-securityDriver
+driver-securityDriver-domainClearSecuritySocketLabel
+driver-securityDriver-domainClearSecuritySocketLabel
+ (driver-securityDriver,vm) 0) {
+VIR_ERROR(_(Failed to clear security context for monitor for %s),
+ vm-def-name);
goto error;
}
--
1.7.1.460.gf3c4c
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] uml_driver: correct logic error in umlMonitorCommand
The stray 0 removed by the fix below
would have made umlMonitorCommand always fail
with its incomplete reply ... diagnostic.
From 5f075332e33f7e9bf2d7979934b8dc7c3312b6e8 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 13 Jul 2010 15:28:35 -0500
Subject: [PATCH] uml_driver: correct logic error in umlMonitorCommand
* src/uml/uml_driver.c (umlMonitorCommand): Correct flaw that would
cause unconditional incomplete reply ... failure, since nbytes
was always 0 or 1.
---
src/uml/uml_driver.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 110179e..1e0f5ac 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -730,7 +730,7 @@ static int umlMonitorCommand(const struct uml_driver
*driver,
ssize_t nbytes;
addrlen = sizeof(addr);
nbytes = recvfrom(priv-monitor, res, sizeof res, 0,
- (struct sockaddr *)addr, addrlen) 0;
+ (struct sockaddr *)addr, addrlen);
if (nbytes 0) {
if (errno == EAGAIN || errno == EINTR)
continue;
--
1.7.1.460.gf3c4c
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] phyp: Adding storage management driver
Justin Clift wrote:
On 06/19/2010 09:33 AM, Eric Blake wrote:
snip
+if (!(profile = phypGetLparProfile(conn, domain-id))) {
+VIR_ERROR(%s, Unable to get VIOS profile name.);
+goto err;
+}
Another case of a missing string translation. How come 'make
syntax-check' isn't catching it?
It also didn't catch some untranslated yes and no strings in
virsh.c that were recently fixed (after being manually noticed).
Guess that needs to be looked at if it's not just virsh that it's not
working correctly on. :/
The sc_libvirt_unmarked_diagnostics rule uses heuristics to detect
uses of error-printing functions -- for which we want translations --
with a literal string argument, but with *no* use of _(...) in the
immediate vicinity.
The heuristics fail when:
- the unmarked string is too far from the function name, either
preceding it, as in the case of yes/no in virsh.c, or after it,
as in two or more lines after the function name.
- there is already one marked string, but there is another that
is not marked.
- a new function of this type is added to libvirt, but its name
is not added to the msg_gen_function list in cfg.mk.
If the penalty for an unmarked string were higher, we would invest more
in automating the check, but for now, manually checking for violations
(while very tedious) will probably turn up more than a few.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix enumeration of partitions in disks with a trailing digit in path
Daniel P. Berrange wrote:
Disks with a trailing digit in their path (eg /dev/loop0 or
/dev/dm0) have an extra 'p' appended before the partition
number (eg, to form /dev/loop0p1 not /dev/loop01). Fix the
partition lookup to append this extra 'p' when required
* src/storage/parthelper.c: Add a 'p' before partition
number if required
---
src/storage/parthelper.c | 15 +++
1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c
index 5626cd2..28d88c9 100644
--- a/src/storage/parthelper.c
+++ b/src/storage/parthelper.c
@@ -36,6 +36,8 @@
#include stdio.h
#include string.h
+#include c-ctype.h
+
/* we don't need to include the full internal.h just for this */
#define STREQ(a,b) (strcmp(a,b) == 0)
@@ -56,6 +58,8 @@ int main(int argc, char **argv)
PedDisk *disk;
PedPartition *part;
int cmd = DISK_LAYOUT;
+const char *path;
+const char *partsep;
if (argc == 3 STREQ(argv[2], -g)) {
cmd = DISK_GEOMETRY;
@@ -64,8 +68,11 @@ int main(int argc, char **argv)
return 1;
}
-if ((dev = ped_device_get(argv[1])) == NULL) {
-fprintf(stderr, unable to access device %s\n, argv[1]);
+path = argv[1];
+partsep = c_isdigit(path[strlen(path)-1]) ? p : ;
Oops.
You'll want to rearrange things (perhaps by rejecting the
empty string early) so that this doesn't reference path[-1]
when given an empty argv[1] (i.e., '').
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3] parthelper: fix compilation without optimization
Eric Blake wrote:
Daniel's patch works with gcc and CFLAGS containing -O (the
autoconf default), but fails with non-gcc or with other
CFLAGS (such as -g), since c-ctype.h declares c_isdigit as
a macro only for certain compilation settings.
* src/Makefile.am (libvirt_parthelper_LDFLAGS): Add gnulib
library, for when c_isdigit is not a macro.
* src/storage/parthelper.c (main): Avoid out-of-bounds
dereference, noticed by Jim Meyering.
---
change in v3: earlier code already guarantees that path is non-null
if we got this far, but not whether it is non-empty
src/Makefile.am |2 +-
src/storage/parthelper.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index 6bdf73c..2129960 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1035,13 +1035,13 @@ if WITH_STORAGE_DISK
if WITH_LIBVIRTD
libexec_PROGRAMS += libvirt_parthelper
libvirt_parthelper_SOURCES = $(STORAGE_HELPER_DISK_SOURCES)
libvirt_parthelper_LDFLAGS = $(WARN_LDFLAGS) $(COVERAGE_LDFLAGS)
libvirt_parthelper_LDADD = $(LIBPARTED_LIBS)
-libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS)
+libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) ../gnulib/lib/libgnu.la
endif
endif
EXTRA_DIST += $(STORAGE_HELPER_DISK_SOURCES)
if WITH_LXC
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c
index 28d88c9..ca74456 100644
--- a/src/storage/parthelper.c
+++ b/src/storage/parthelper.c
@@ -66,13 +66,13 @@ int main(int argc, char **argv)
} else if (argc != 2) {
fprintf(stderr, syntax: %s DEVICE [-g]\n, argv[0]);
return 1;
}
path = argv[1];
-partsep = c_isdigit(path[strlen(path)-1]) ? p : ;
+partsep = *path c_isdigit(path[strlen(path)-1]) ? p : ;
if ((dev = ped_device_get(path)) == NULL) {
fprintf(stderr, unable to access device %s\n, path);
return 2;
}
Looks good.
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] make syntax-check error due to bad email addr
Justin Clift wrote: make syntax-check is reporting an error with the AUTHORS file, which seems to be coming from an incorrect address for Jim Fehlig. ... check_author_list jfeh...@linux-ypgk.site maint.mk: committer(s) not listed in AUTHORS make: *** [sc_check_author_list] Error 1 $ This is the git log entry it may be from: commit b1eb7f2e987d21b1711e86e5cb63a69abfce82f1 Author: Jim Fehlig jfeh...@linux-ypgk.site ... Adding the incorrect email to the AUTHORS file is a workaround, but is someone ok to fix the cause? Thanks for reporting that. I've fixed it by pushing this change: From ef77388ba4ba0bf37d426a7c2f7a983571acc3ea Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Sun, 6 Jun 2010 10:27:28 +0200 Subject: [PATCH] avoid syntax-check failure * .mailmap: Map a stray commit-author email address to the canonical one. Reported by Justin Clift. --- .mailmap |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/.mailmap b/.mailmap index dff04b9..8c1eed3 100644 --- a/.mailmap +++ b/.mailmap @@ -10,3 +10,4 @@ meyer...@redhat.com j...@meyering.net socketp...@gmail.com socketpair gmail com so...@canonical.com so...@ubuntu.com +jfeh...@novell.com jfeh...@linux-ypgk.site -- 1.7.1.445.gc54f1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Allocate buffer to hold xend response
Jim Fehlig wrote:
Eric Blake wrote:
BTW, I pushed the patch after Eric's ACK. I'll role another to address
these issues, once I get confirmation on the NUL-termination.
Looking forward to the followup, and thanks for Jim for catching
something I missed in my ACK (even if we didn't catch it in time).
Patch that includes Jim's suggestions ...
...
Subject: [PATCH] Fixes for commit 211dd1e9
Fixes for issues in commit 211dd1e9 noted by by Jim Meyering.
1. Allocate content buffer of size content_length + 1 to ensure
NUL-termination.
2. Limit content buffer size to 64k
3. Fix whitespace issue
---
src/xen/xend_internal.c | 14 --
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 0c1a738..3b9da6b 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -68,6 +68,7 @@
# define XEND_CONFIG_MIN_VERS_PVFB_NEWCONF 3
#endif
+#define XEND_RCV_BUF_MAX_LEN 65536
#ifndef PROXY
static int
@@ -330,7 +331,16 @@ xend_req(int fd, char **content)
if (content_length 0) {
ssize_t ret;
Hi Jim,
Thanks for adding that.
This part looks fine.
-if (VIR_ALLOC_N(*content, content_length) 0 ) {
+if (content_length XEND_RCV_BUF_MAX_LEN) {
+virXendError(VIR_ERR_INTERNAL_ERROR,
+ _(Xend returned HTTP Content-Length of %d,
+ which exceeds maximum of %d),
+ content_length,
+ XEND_RCV_BUF_MAX_LEN);
+return -1;
+}
+
This is subtle enough that a comment might avoid trouble later.
/* Allocate one byte beyond the end of the largest buffer we will read.
Combined with the fact that VIR_ALLOC_N zeros the returned buffer,
this guarantees that content will always be NUL-terminated. */
+if (VIR_ALLOC_N(*content, content_length + 1) 0 ) {
virReportOOMError();
return -1;
}
@@ -380,7 +390,7 @@ xend_get(virConnectPtr xend, const char *path,
virXendError(VIR_ERR_GET_FAILED,
_(%d status from xen daemon: %s:%s),
ret, path,
- content ? *content: NULL);
+ content ? *content : NULL);
Oh! I've just noticed that testing content is wrong,
since it will always be non-NULL. BTW, the parameter should
be marked as such via ATTRIBUTE_NONNULL (3).
The test should examine *content, i.e.,
*content ? *content : NULL);
Then I remembered the NULLSTR macro.
You can replace the above with this:
NULLSTR(*content));
FYI, here's its definition:
$ git grep -A2 ine.NULLSTR
src/internal.h:# define NULLSTR(s) \
src/internal.h-((void)verify_true(sizeof *(s) == sizeof (char)), \
src/internal.h- (s) ? (s) : (null))
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Allocate buffer to hold xend response
Jim Fehlig wrote: ... Subject: [PATCH] Fixes for commit 211dd1e9 Fixes for issues in commit 211dd1e9 noted by by Jim Meyering. 1. Allocate content buffer of size content_length + 1 to ensure NUL-termination. 2. Limit content buffer size to 64k 3. Fix whitespace issue V2: - Add comment to clarify allocation of content buffer - Add ATTRIBUTE_NONNULL where appropriate - User NULLSTR macro ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Allocate buffer to hold xend response
Jim Fehlig wrote:
...
Subject: [PATCH] Allocate buffer to hold xend response
There are cases when a response from xend can exceed 4096 bytes, in
which case anything beyond 4096 is ignored. This patch changes the
current fixed-size, stack-allocated buffer to a dynamically allocated
buffer based on Content-Length in HTTP header.
---
src/xen/xend_internal.c | 80
---
1 files changed, 41 insertions(+), 39 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index e763bad..0c1a738 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -302,17 +302,19 @@ istartswith(const char *haystack, const char *needle)
* xend_req:
* @fd: the file descriptor
* @content: the buffer to store the content
- * @n_content: the size of the buffer
*
* Read the HTTP response from a Xen Daemon request.
+ * If the response contains content, memory is allocated to
+ * hold the content.
*
- * Returns the HTTP return code.
+ * Returns the HTTP return code and @content is set to the
+ * allocated memory containing HTTP content.
*/
static int
-xend_req(int fd, char *content, size_t n_content)
+xend_req(int fd, char **content)
{
char buffer[4096];
-int content_length = -1;
+int content_length = 0;
int retcode = 0;
while (sreads(fd, buffer, sizeof(buffer)) 0) {
@@ -325,19 +327,17 @@ xend_req(int fd, char *content, size_t n_content)
retcode = atoi(buffer + 9);
}
-if (content_length -1) {
+if (content_length 0) {
ssize_t ret;
-if ((unsigned int) content_length (n_content + 1))
-content_length = n_content - 1;
+if (VIR_ALLOC_N(*content, content_length) 0 ) {
+virReportOOMError();
+return -1;
+}
-ret = sread(fd, content, content_length);
+ret = sread(fd, *content, content_length);
if (ret 0)
return -1;
-
-content[ret] = 0;
-} else {
-content[0] = 0;
}
Hi Jim,
The above removes the code that guarantees the content buffer is
NUL-terminated, yet I don't see where the requirement for NUL-termination
has been dropped. Even if the content payload we receive typically ends
in a NUL, we cannot guarantee that (i.e., content may be corrupted or
malicious), so we have to verify the assumption or add a NUL byte of
our own.
While your change does adjust the xend_req caller to handle NULL
content, here it will read beyond end of buffer when *content has
no trailing NUL byte:
virXendError(VIR_ERR_GET_FAILED,
_(%d status from xen daemon: %s:%s),
- ret, path, content);
+ ret, path,
+ content ? *content: NULL);
BTW, please add a space before the :.
Also, now that we'll get the buffer length from an untrusted
source, it would be good to ensure that it's not outrageously
large before using it as the size of the buffer we allocate,
assuming there is some reasonably low upper bound on the maximum
payload size. That could save us from potential DoS abuse.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v13] [REPOST] add 802.1Qbh and 802.1Qbg handling
Stefan Berger wrote:
Formatting errors in the previous posting (and another problem).
This is now hopefully the final version of this patch that adds support
for configuring 802.1Qbg and 802.1Qbh switches. The 802.1Qbh part has
been successfully tested with real hardware. The 802.1Qbg part has only
been tested with a (dummy) server that 'behaves' similarly to how we
expect lldpad to 'behave'.
V13:
- Merging Scott's v13-pre1 patch
- Fixing endptr related bug while using virStrToLong_ui() pointed out
by Jim Meyering
...
+static uint32_t
+getLldpadPid(void) {
+int fd;
+uint32_t pid = 0;
+
+fd = open(LLDPAD_PID_FILE, O_RDONLY);
+if (fd = 0) {
+char buffer[10];
+char *endptr;
+
+if (saferead(fd, buffer, sizeof(buffer)) = sizeof(buffer)) {
+unsigned int res;
+
+if (virStrToLong_ui(buffer, endptr, 10, res) == 0
+ (endptr == NULL || c_isspace(*endptr))
+ res != 0) {
+pid = res;
+} else {
+macvtapError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(error parsing pid of lldpad));
+}
+}
+} else {
+virReportSystemError(errno,
+ _(Error opening file %s), LLDPAD_PID_FILE);
+}
+
+if (fd = 0)
+close(fd);
+
+return pid;
+}
Hi Stefan,
Sorry, but my suggestion was incomplete. *endptr == '\0' indicates
a valid conversion, too. That's what would happen if there's no byte
at all (not even a trailing newline) after the PID. In addition, when
virStrToLong_ui returns 0, endptr cannot be NULL, so there was no need
for that test. This handles it:
(*endptr == '\0' || c_isspace(*endptr))
With that, ACK.
A minor improvement: move the declarations of buffer and endptr
down into the if (saferead(... block alongside res, since that's
the only scope in which they're used.
The rest looked fine, too -- but I don't know enough
to spot 802.1Qbx protocol bugs.
Jim
P.S. There are probably enough PID-reading blocks of
code like this that it'd be worth factoring this into
a function in util.c.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] build: make cpp indentation conform
FYI, I've pushed this:
From f9a4df5a5bc4ab9a645928430db84ec3807a996e Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sat, 29 May 2010 09:45:21 +0200
Subject: [PATCH] build: make cpp indentation conform
* src/storage/storage_backend.h (VIR_STORAGE_VOL_OPEN_DEFAULT):
Adjust s/#define/# define/, and align continued lines.
---
src/storage/storage_backend.h |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index bb4d1c0..1165a45 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -93,10 +93,10 @@ enum {
VIR_STORAGE_VOL_OPEN_CHAR = 1 3, /* char files okay */
};
-#define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_ERROR|\
- VIR_STORAGE_VOL_OPEN_REG |\
- VIR_STORAGE_VOL_OPEN_CHAR |\
- VIR_STORAGE_VOL_OPEN_BLOCK)
+# define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_ERROR|\
+ VIR_STORAGE_VOL_OPEN_REG |\
+ VIR_STORAGE_VOL_OPEN_CHAR |\
+ VIR_STORAGE_VOL_OPEN_BLOCK)
int virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags)
ATTRIBUTE_RETURN_CHECK
--
1.7.1.348.gb26ba
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v11] add 802.1Qbh and 802.1Qbg handling
Stefan Berger wrote: On Fri, 2010-05-28 at 23:17 +0200, Jim Meyering wrote: Stefan Berger wrote: This is now hopefully the final version of this patch that adds support for configuring 802.1Qbg and 802.1Qbh switches. The 802.1Qbh part has been successfully tested with real hardware. The 802.1Qbg part has only been tested with a (dummy) server that 'behaves' similarly to how we expect lldpad to 'behave'. V11: - determining pid of lldpad daemon by reading it from /var/run/libvirt.pid (hardcode as is hardcode alson in lldpad sources) - merging netlink send code for kernel target and user space target (lldpad) using one function nlComm() to send the messages - adding a select() after the sending and before the reading of the netlink response in case lldpad doesn't respond and so we don't hang - when reading the port status, in case of 802.1Qbg, no status may be received while things are 'in progress' and only at the end a status will be there. - when reading the port status, use the given instanceId and vf to pick the right IFLA_VF_PORT among those nested under IFLA_VF_PORTS. Hi Stefan, There are a few nits, but nothing serious. If this will be pushed in your name, you should add your name/email to the AUTHORS file. Already there... Oh, I see what happened. The problem I saw was when using git-am to apply that, it used stefanb-at-linux.vnet.ibm.com (with @, of course) as your address, and that address is not in AUTHORS (there, it's @ibm...), so the test failed. Assuming you push with the email address that is already in AUTHORS, there will be no failure. There are two cpp-indentation nits: cppi: src/storage/storage_backend.h: line 96: not properly indented cppi: src/util/macvtap.c: line 75: not properly indented And one unmarked diagnostic: src/util/macvtap.c-766-error parsing pid of lldpad); Those three things were exposed by running make syntax-check. Fixed the two related to this patch. The storage one didn't appear. It's independent of your change. I've fixed it. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v12] add 802.1Qbh and 802.1Qbg handling
Stefan Berger wrote:
...
V12:
- Addressing Jim Meyering's comments to v11
- requiring mac address to the vpDisassociateProfileId() function to
pass it further to the 802.1Qbg disassociate part (802.1Qbh untouched)
Thanks for enduring so many iterations.
...
+static uint32_t
+getLldpadPid(void) {
+int fd;
+uint32_t pid = 0;
+
+fd = open(LLDPAD_PID_FILE, O_RDONLY);
+if (fd = 0) {
+char buffer[10];
+char *endptr;
+
+if (saferead(fd, buffer, sizeof(buffer)) = sizeof(buffer)) {
+unsigned int res;
+
+if (virStrToLong_ui(buffer, endptr, 10, res)
+(endptr == NULL || c_isspace(*endptr)))
+macvtapError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(error parsing pid of lldpad));
+else
+pid = res;
+}
That new (...) conjunct is wrong, since it makes the code
check endptr only when parsing fails.
Writing || !(...) would have worked but is harder to read than...
How about this, reversing the if/else blocks?
Also, this adds a test to diagnose a PID with value 0 as invalid
(which would indicate failure with no diagnostic)
and adjusts the diagnostic accordingly, since that is not a parse error:
if (virStrToLong_ui(buffer, endptr, 10, res) == 0
(endptr == NULL || c_isspace(*endptr))
res != 0)
pid = res;
else
macvtapError(VIR_ERR_INTERNAL_ERROR, %s,
_(invalid lldpad PID));
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] another AUTHORS update
Eric Blake wrote:
Eduardo Otubo contacted me off-list, noticing that his name was not yet
in AUTHORS even though he has had commits in the past. I've rectified
this situation with an obvious commit; not worth posting the diff to the
list (to avoid unnecessary exposure of all the email addresses...).
Here's a rule to help us automate the task of keeping
our AUTHORS file in sync with the commit history:
[I know some are sensitive about having their email
addresses appear in the clear, so I've manually mangled
the .mailmap contents in this message by filtering it through rot13.
Obviously, .mailmap will not be obfuscated in the repository. ]
From e19a035f7c1a4ca5fcdcc58033b8a1c42ef19fa9 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 28 May 2010 11:27:12 +0200
Subject: [PATCH] maint: new syntax-check rule to ensure that AUTHORS stays in
sync
* cfg.mk (sc_check_author_list): New rule.
* .mailmap: New file, to tell git log how to map email addresses.
---
.mailmap | 12
cfg.mk | 14 ++
2 files changed, 26 insertions(+), 0 deletions(-)
create mode 100644 .mailmap
diff --git a/.mailmap b/.mailmap
new file mode 100644
index 000..dff04b9
--- /dev/null
+++ b/.mailmap
@@ -0,0 +1,12 @@
+nzl.tevs...@uc.pbz neba.tevs...@uc.pbz
+obmmb...@tznvy.pbz erqfu...@tzk.pbz
+puneyrf_qh...@zrffntrbar.pbz pune...@qlsvf.arg
+q...@erqung.pbz q...@qsw.oar.erqung.pbz
+roy...@erqung.pbz r...@olh.arg
+tqby...@necargjbexf.pbz tqby...@hpyn.rqh
+treuneq.fgra...@qr.voz.pbz tfgra...@yvahk.iarg.voz.pbz
+wn...@pnabavpny.pbz wn...@hohagh.pbz
+yn...@erqung.pbz yn...@ynvar.bet
+zrlre...@erqung.pbz w...@zrlrevat.arg
+fbpxrgc...@tznvy.pbz fbpxrgcnve tznvy pbz
+fb...@pnabavpny.pbz fb...@hohagh.pbz
diff --git a/cfg.mk b/cfg.mk
index bdf9ea9..e602df2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -472,6 +472,20 @@ Makefile: _autogen
endif
endif
+# Give credit where due:
+# Ensure that each commit author email address (possibly mapped via
+# git log's .mailmap) appears in our AUTHORS file.
+sc_check_author_list:
+ @fail=0;\
+ for i in $$(git log --pretty=format:%aE%n|sort -u|grep -v '^$$'); do \
+ sanitized=$$(echo $$i|LC_ALL=C sed 's/\([^a-za-z0...@-]\)/\\\1/g');
\
+ grep -iq $$sanitized AUTHORS \
+ || { printf '%s\n' $$i 2; fail=1; };\
+ done; \
+ test $$fail = 1 \
+ echo '$(ME): committer(s) not listed in AUTHORS' 2; \
+ test $$fail = 0
+
# It is necessary to call autogen any time gnulib changes. Autogen
# reruns configure, then we regenerate all Makefiles at once.
.PHONY: _autogen
--
1.7.1.348.gb26ba
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] another AUTHORS update
Eric Blake wrote:
On 05/28/2010 03:33 AM, Jim Meyering wrote:
Eric Blake wrote:
Eduardo Otubo contacted me off-list, noticing that his name was not yet
in AUTHORS even though he has had commits in the past. I've rectified
this situation with an obvious commit; not worth posting the diff to the
list (to avoid unnecessary exposure of all the email addresses...).
Here's a rule to help us automate the task of keeping
our AUTHORS file in sync with the commit history:
Cool idea!
+# Give credit where due:
+# Ensure that each commit author email address (possibly mapped via
+# git log's .mailmap) appears in our AUTHORS file.
+sc_check_author_list:
+@fail=0;\
+for i in $$(git log --pretty=format:%aE%n|sort -u|grep -v '^$$'); do \
+ sanitized=$$(echo $$i|LC_ALL=C sed 's/\([^a-za-z0...@-]\)/\\\1/g');
\
+ grep -iq $$sanitized AUTHORS \
+|| { printf '%s\n' $$i 2; fail=1; };\
+done; \
+test $$fail = 1 \
+ echo '$(ME): committer(s) not listed in AUTHORS' 2; \
+test $$fail = 0
ACK
Thanks. Pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v11] add 802.1Qbh and 802.1Qbg handling
Stefan Berger wrote:
This is now hopefully the final version of this patch that adds support
for configuring 802.1Qbg and 802.1Qbh switches. The 802.1Qbh part has
been successfully tested with real hardware. The 802.1Qbg part has only
been tested with a (dummy) server that 'behaves' similarly to how we
expect lldpad to 'behave'.
V11:
- determining pid of lldpad daemon by reading it from /var/run/libvirt.pid
(hardcode as is hardcode alson in lldpad sources)
- merging netlink send code for kernel target and user space target
(lldpad) using one function nlComm() to send the messages
- adding a select() after the sending and before the reading of the
netlink response in case lldpad doesn't respond and so we don't hang
- when reading the port status, in case of 802.1Qbg, no status may be
received while things are 'in progress' and only at the end a status
will be there.
- when reading the port status, use the given instanceId and vf to pick
the right IFLA_VF_PORT among those nested under IFLA_VF_PORTS.
Hi Stefan,
There are a few nits, but nothing serious.
If this will be pushed in your name, you should add
your name/email to the AUTHORS file.
There are two cpp-indentation nits:
cppi: src/storage/storage_backend.h: line 96: not properly indented
cppi: src/util/macvtap.c: line 75: not properly indented
And one unmarked diagnostic:
src/util/macvtap.c-766-error parsing pid of lldpad);
Those three things were exposed by running make syntax-check.
Further, in this code,
+static uint32_t
+getLldpadPid(void) {
+int fd;
+uint32_t pid = 0;
+
+fd = open(LLDPAD_PID_FILE, O_RDONLY);
+if (fd = 0) {
+char buffer[10];
+char *endptr;
+
+if (saferead(fd, buffer, sizeof(buffer)) = sizeof(buffer)) {
+unsigned int res;
+
+if (virStrToLong_ui(buffer, endptr, 10, res))
+macvtapError(VIR_ERR_INTERNAL_ERROR, %s,
+ error parsing pid of lldpad);
+else
+pid = res;
by passing a non-NULL endptr (and not checking it after the call),
you're declaring that any non-numeric suffix on that PID is valid.
It would be better not to do that: it'd be better to ensure that
anything following it is acceptable, e.g. via
endptr == NULL || c_isspace(endptr)
Or, if you can guarantee how it's written, simply require that it
be followed by a newline:
endptr *endptr == '\n'
Stylistic:
In this function,
+static int
+doPortProfileOpCommon(bool nltarget_kernel,
you add this loop:
+while (--repeats = 0) {
+rc = link_dump(nltarget_kernel, NULL, ifindex, tb, recvbuf);
+if (rc)
+goto err_exit;
+rc = getPortProfileStatus(tb, vf, instanceId, nltarget_kernel,
+ is8021Qbg, status);
+if (rc == 0) {
+if (status == PORT_PROFILE_RESPONSE_SUCCESS ||
+status == PORT_VDP_RESPONSE_SUCCESS) {
+break;
+} else if (status == PORT_PROFILE_RESPONSE_INPROGRESS) {
+// keep trying...
+} else {
+virReportSystemError(EINVAL,
+_(error %d during port-profile setlink on
+ interface %s (%d)),
+status, ifname, ifindex);
+rc = 1;
+break;
+}
+} else
+goto err_exit;
+
+usleep(STATUS_POLL_INTERVL_USEC);
+
+VIR_FREE(recvbuf);
+}
However, I find it simpler to read if you put the
single-stmt-goto-in-else-clause first,
and then un-indent what is currently the body of that if block:
+if (rc != 0)
+goto err_exit;
+
+if (status == PORT_PROFILE_RESPONSE_SUCCESS ||
+status == PORT_VDP_RESPONSE_SUCCESS) {
+break;
+} else if (status == PORT_PROFILE_RESPONSE_INPROGRESS) {
+// keep trying...
+} else {
+virReportSystemError(EINVAL,
+_(error %d during port-profile setlink on
+ interface %s (%d)),
+status, ifname, ifindex);
+rc = 1;
+break;
+}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] network: bridge: Don't start network if it collides with host routing
Cole Robinson wrote:
Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961
If using the default virtual network, an easy way to lose guest network
connectivity is to install libvirt inside the VM. The autostarted
default network inside the guest collides with host virtual network
routing. This is a long standing issue that has caused users quite a
bit of pain and confusion.
On network startup, parse /proc/net/route and compare the requested
IP+netmask against host routing destinations: if any matches are found,
refuse to start the network.
v2: Drop sscanf, fix a comment typo, comment that function could use
libnl instead of /proc
v3: Consider route netmask. Compare binary data rather than convert to
string.
...
+static int networkCheckRouteCollision(virNetworkObjPtr network)
+{
+int ret = -1, len;
+char *cur, *buf = NULL;
+enum {MAX_ROUTE_SIZE = 1024*64};
+struct in_addr inaddress, innetmask;
+char netaddr[32];
+
+if (!network-def-ipAddress || !network-def-netmask)
+return 0;
+
+if (inet_pton(AF_INET, network-def-ipAddress, inaddress) = 0) {
+networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _(cannot parse IP address '%s'),
+ network-def-ipAddress);
+goto error;
+}
+if (inet_pton(AF_INET, network-def-netmask, innetmask) = 0) {
+networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _(cannot parse netmask '%s'),
+ network-def-netmask);
+goto error;
+}
+
+inaddress.s_addr = innetmask.s_addr;
+if (!inet_ntop(AF_INET, inaddress, netaddr, sizeof(netaddr))) {
+virReportSystemError(errno, %s,
+ _(failed to format network address));
+goto error;
+}
+
+/* Read whole routing table into memory */
+if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, buf)) 0)
+goto error;
+/* Dropping the last character shouldn't hurt */
Hi Cole,
Not sure how much you want to invest in manual parsing,
but in case this code ends up staying with us for a while,
here are some suggestions.
Handle the case in which the file is empty.
This will appease static checkers like clang and coverity.
Either change the 0 test above to = 0
or do e.g.,
if (len 0)
+buf[len-1] = '\0';
Future proof it by skipping that first line only
if it looks like the heading we see now:
if (STRPREFIX (buf, Iface))
+/* First line is just headings, skip it */
+cur = strchr(buf, '\n');
+
+while (cur) {
+char *data[8];
+char *dest, *iface, *mask;
+unsigned int addr_val, mask_val;
+int i;
It's slightly better to make i unsigned.
The following assumes no leading white space, which is currently true
at least on F13. Future/portability-proof it by skipping leading white space:
while (c_isblank (*cur))
cur++;
+cur++;
+
+/* Delimit interface field */
+for (i = 0; i sizeof(data); ++i) {
Oops. Won't this make i iterate from 0 up to 31 or 63,
depending on sizeof char*?
You probably mean this:
for (i = 0; i ARRAY_CARDINALITY(data); ++i) {
+data[i] = cur;
Otherwise, this would overrun the 8-element data array
and clobber the stack.
+/* Parse fields and delimit */
+while(*cur ' ') {
Using ' ' works as long as each line has 8 or more fields.
If there are fewer, it would treat the newline just like a regular field
separator and continue getting fields from the next line.
If you use c_isblank you have to handle end of line differently,
but that's a good thing.
How about using c_isblank here, too, rather than relying on
ASCII space being smaller than interesting byte codes?
On F13, the fields of /proc/net/route are TAB-delimited.
Besides, I think y
while (*cur !c_isblank (*cur)) {
+cur++;
+}
+*cur++ = '\0';
You'll want something here to keep cur from going more than 1 beyond
the end of the buffer, in case the last row has fewer than 8 columns.
+}
+
+iface = data[0];
+dest = data[1];
+mask = data[7];
If a line had fewer than 8 columns, at least mask is not valid.
Similarly for dest if there are fewer than two columns.
+if (virStrToLong_ui(dest, NULL, 16, addr_val) 0) {
+networkReportError(VIR_ERR_INTERNAL_ERROR,
+ _(Failed to convert network address %s),
+ dest);
+goto error;
+}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: udev: Fix handling of wireless NIC
Cole Robinson wrote:
Wireless NICs were being ignored because we weren't correctly handling
device type. Fix this, as well as wireless NIC net subtype.
Signed-off-by: Cole Robinson crobi...@redhat.com
---
src/node_device/node_device_udev.c | 12 +++-
1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index f0485f1..4915d4e 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -597,8 +597,16 @@ static int udevProcessNetworkInterface(struct
udev_device *device,
virNodeDeviceDefPtr def)
{
int ret = -1;
+const char *devtype = NULL;
union _virNodeDevCapData *data = def-caps-data;
+devtype = udev_device_get_devtype(device);
You can combine the declaration and first assignment:
const char *devtype = udev_device_get_devtype(device);
to avoid a dead store (clang and coverity complain about those)
and eliminate the minor duplication of that variable name.
+if (devtype STREQ(devtype, wlan)) {
+data-net.subtype = VIR_NODE_DEV_CAP_NET_80211;
+} else {
+data-net.subtype = VIR_NODE_DEV_CAP_NET_80203;
+}
+
if (udevGetStringProperty(device,
INTERFACE,
data-net.ifname) == PROPERTY_ERROR) {
@@ -1074,6 +1082,8 @@ static int udevGetDeviceType(struct udev_device *device,
int ret = 0;
devtype = udev_device_get_devtype(device);
+VIR_DEBUG(Found device type '%s' for device '%s',
+ devtype, udev_device_get_sysname(device));
if (devtype != NULL STREQ(devtype, usb_device)) {
*type = VIR_NODE_DEV_CAP_USB_DEV;
@@ -1112,7 +1122,7 @@ static int udevGetDeviceType(struct udev_device *device,
/* It does not appear that network interfaces set the device type
* property. */
-if (devtype == NULL
+if ((devtype == NULL || STREQ(devtype, wlan))
udevGetStringProperty(device,
INTERFACE,
tmp_string) == PROPERTY_FOUND) {
Otherwise, this looks fine.
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] xen: Fix chardev listen sexpr formatting
Cole Robinson wrote: 'listen' isn't a valid qemu-dm option, as reported a long time ago here: https://bugzilla.redhat.com/show_bug.cgi?id=492958 Matches the near identical logic in qemu_conf.c ... diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index a99cc7b..e12bac7 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1276,7 +1276,7 @@ xenDaemonParseSxprChar(const char *value, if (def-data.tcp.service == NULL) goto no_memory; -if (offset2 strstr(offset2, ,listen)) +if (offset2 strstr(offset2, ,server,nowait)) def-data.tcp.listen = 1; } break; @@ -1332,7 +1332,7 @@ xenDaemonParseSxprChar(const char *value, goto no_memory; if (offset != NULL -strstr(offset, ,listen) != NULL) +strstr(offset, ,server,nowait) != NULL) def-data.nix.listen = 1; } break; @@ -5209,7 +5209,7 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, tcp : telnet), (def-data.tcp.host ? def-data.tcp.host : ), (def-data.tcp.service ? def-data.tcp.service : ), - (def-data.tcp.listen ? ,listen : )); + (def-data.tcp.listen ? ,server,nowait : )); break; case VIR_DOMAIN_CHR_TYPE_UDP: @@ -5223,7 +5223,7 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: virBufferVSprintf(buf, %s:%s%s, type, def-data.nix.path, - def-data.nix.listen ? ,listen : ); + def-data.nix.listen ? ,server,nowait : ); ACK on the basis of similarity to existing code in qemu_conf.c as you noted. However, is it possible to encounter the option names ordered the other way: ,nowait,server -- or even with some unrelated option between them ? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: udev: Fix handling of wireless NIC
Dave Allan wrote:
On Wed, May 26, 2010 at 03:55:44PM -0400, Cole Robinson wrote:
Wireless NICs were being ignored because we weren't correctly handling
device type. Fix this, as well as wireless NIC net subtype.
Thanks for finding and fixing this. I made a v2 patch with the change
that since DEVTYPE == wlan is a definitive indication that the
device is a network interface, that case should be with the other
device types for which DEVTYPE is definitive. v2 patch attached.
Dave
From ccf89c57a6335c1c7c7e0b29ae09f4916b33622d Mon Sep 17 00:00:00 2001
From: David Allan dal...@redhat.com
Date: Thu, 27 May 2010 10:44:02 -0400
Subject: [PATCH 1/1] v2 of Cole's wlan support
* Incorporated Jim's feedback
* Moved case of DEVTYPE == wlan up as it's definitive that we have a
network interface.
* Made comment more detailed about the wired case to explain better
how it differentiates between wired network interfaces and USB
devices.
...
int ret = -1;
Thanks Dave.
+const char *devtype = udev_device_get_devtype(device);
union _virNodeDevCapData *data = def-caps-data;
+if (devtype STREQ(devtype, wlan)) {
+data-net.subtype = VIR_NODE_DEV_CAP_NET_80211;
+} else {
+data-net.subtype = VIR_NODE_DEV_CAP_NET_80203;
+}
+
if (udevGetStringProperty(device,
INTERFACE,
data-net.ifname) == PROPERTY_ERROR) {
@@ -1074,6 +1081,8 @@ static int udevGetDeviceType(struct udev_device *device,
int ret = 0;
devtype = udev_device_get_devtype(device);
+VIR_DEBUG(Found device type '%s' for device '%s',
+ devtype, udev_device_get_sysname(device));
Sorry I missed it the first time, but since devtype can be
NULL here, we have to avoid dereferencing it:
(it wouldn't cause trouble with glibc, but would segfault on others)
VIR_DEBUG(Found device type '%s' for device '%s',
NULLSTR(devtype), udev_device_get_sysname(device));
if (devtype != NULL STREQ(devtype, usb_device)) {
*type = VIR_NODE_DEV_CAP_USB_DEV;
@@ -1105,17 +1114,25 @@ static int udevGetDeviceType(struct udev_device
*device,
goto out;
}
+if (devtype != NULL STREQ(devtype, wlan)) {
+*type = VIR_NODE_DEV_CAP_NET;
+goto out;
+}
+
if (udevGetUintProperty(device, PCI_CLASS, tmp, 16) ==
PROPERTY_FOUND) {
*type = VIR_NODE_DEV_CAP_PCI_DEV;
goto out;
}
-/* It does not appear that network interfaces set the device type
- * property. */
-if (devtype == NULL
+/* It does not appear that wired network interfaces set the
+ * DEVTYPE property. USB devices also have an INTERFACE property,
+ * but they do set DEVTYPE, so if devtype is NULL and the
+ * INTERFACE property exists, we have a network device. */
+if ((devtype == NULL)
I wouldn't bother to add parens above.
udevGetStringProperty(device,
INTERFACE,
tmp_string) == PROPERTY_FOUND) {
+
IMHO, that added blank line detracts from readability.
VIR_FREE(tmp_string);
*type = VIR_NODE_DEV_CAP_NET;
goto out;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxcSetSchedulerParameters: reverse order of tests; improve a diagnostic
Eric Blake wrote: On 05/12/2010 07:23 PM, Eric Blake wrote: [trying to clear out some old mail] From bc3404d9f12c42cf883a43395fee6fc14c952b2c Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Tue, 11 May 2010 15:43:32 +0200 Subject: [PATCH] lxcSetSchedulerParameters: reverse order of tests; diagnose a failure * src/lxc/lxc_driver.c (lxcSetSchedulerParameters): Ensure that -field is cpu_shares before possibly giving a diagnostic about a type for a cpu_shares value. Also, virCgroupSetCpuShares could fail without evoking a diagnostic. Add one. lxcError(VIR_ERR_INVALID_ARG, %s, - _(Invalid type for cpu_shares tunable, expected a 'ullong')); + _(Invalid type for cpu_shares tunable, expected a 'ullong')); Why the indentation change here? To answer my own question - to fit in 80 columns. ACK. I applied this one on Jim's behalf, after validating that everything still builds fine after rebasing on current sources. Thanks! It'd fallen off my radar. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [v5 PATCH] add 802.1Qbh handling for port-profiles based on Stefan's previous patches
Scott Feldman wrote:
From: Scott Feldman scofe...@cisco.com
...
diff --git a/configure.ac b/configure.ac
...
-if test $with_macvtap = yes; then
+if test $with_macvtap = yes || $with_virtualport = yes; then
Hi Scott,
The above introduces a syntax error.
Fix it by inserting a test after the ||:
if test $with_macvtap = yes || test $with_virtualport = yes; then
...
diff --git a/src/util/macvtap.c b/src/util/macvtap.c
...
@@ -159,6 +170,156 @@ err_exit:
}
+# ifdef IFLA_VF_PORT_MAX
+
+/**
+ * nlCommWaitSuccess:
+ *
+ * @nlmsg: pointer to netlink message
+ * @nl_grousp: the netlink multicast groups to send to
+ * @respbuf: pointer to pointer where response buffer will be allocated
+ * @respbuflen: pointer to integer holding the size of the response buffer
+ * on return of the function.
+ * @to_usecs: timeout in microseconds to wait for a success message
+ *to be returned
+ *
+ * Send the given message to the netlink multicast group and receive
+ * responses. Skip responses indicating an error and keep on receiving
+ * responses until a success response is returned.
+ * Returns 0 on success, -1 on error. In case of error, no response
+ * buffer will be returned.
+ */
+static int
+nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
+ char **respbuf, int *respbuflen, long to_usecs)
The parameters, respbuflen and nl_groups make sense only when
non-negative, so my reflex is to make their types reflect that.
Any reason not to use an unsigned type in those cases?
This to_usecs parameter is in the same boat, in that it is
logically non-negative. I suggest using unsigned long long
for it, since technically (with the two longs in struct timeval)
a portable timeout can be as large as 2^31 * 1000^2 microseconds.
Also, it'd be nice to rename it to timeout_usecs, since to_usecs
made me think of a flag that says whether to convert to microseconds.
+{
+int rc = 0;
+struct sockaddr_nl nladdr = {
+.nl_family = AF_NETLINK,
+.nl_pid= getpid(),
+.nl_groups = nl_groups,
+};
+int rcvChunkSize = 1024; // expecting less than that
+int rcvoffset = 0;
This should be of type size_t.
It is used as an accumulator, and we do add nbytes (of type ssize_t) to it,
and (esp!) use it as an array index, so its type must be unsigned.
Otherwise, overflow could be exploitable.
+ssize_t nbytes;
+int n;
+struct timeval tv = {
+.tv_sec = to_usecs / MICROSEC_PER_SEC,
+.tv_usec = to_usecs % MICROSEC_PER_SEC,
+};
+fd_set rfds;
At least n and rfds are used only in the while loop,
so their declarations belong down there, too.
+bool gotvalid = false;
Personal preference:
I find that a name like got_valid is easier to read than a
variablenamewithnoseparatorbetweenwords.
Same for rcvoffset vs. rcv_offset, etc.
+int fd = nlOpen();
+static uint32_t seq = 0x1234;
+uint32_t myseq = seq++;
+uint32_t mypid = getpid();
+
+if (fd 0)
+return -1;
+
+nlmsg-nlmsg_pid = mypid;
+nlmsg-nlmsg_seq = myseq;
+nlmsg-nlmsg_flags |= NLM_F_ACK;
+
+nbytes = sendto(fd, (void *)nlmsg, nlmsg-nlmsg_len, 0,
+(struct sockaddr *)nladdr, sizeof(nladdr));
+if (nbytes 0) {
+virReportSystemError(errno,
+ %s, _(cannot send to netlink socket));
+rc = -1;
+goto err_exit;
+}
+
+while (!gotvalid) {
+rcvoffset = 0;
+while (1) {
+socklen_t addrlen = sizeof(nladdr);
+
+if (VIR_REALLOC_N(*respbuf, rcvoffset+rcvChunkSize) 0) {
+virReportOOMError();
+rc = -1;
+goto err_exit;
+}
+
+FD_ZERO(rfds);
+FD_SET(fd, rfds);
+
+n = select(fd + 1, rfds, NULL, NULL, tv);
+if (n == 0) {
That handles the case of an expired timeout.
However, if select fails, it returns -1.
You should diagnose that failure rather than going
ahead and reading from fd.
+rc = -1;
+goto err_exit;
+}
+
+nbytes = recvfrom(fd, ((*respbuf)[rcvoffset]), rcvChunkSize, 0,
+ (struct sockaddr *)nladdr, addrlen);
+if (nbytes 0) {
+if (errno == EAGAIN || errno == EINTR)
+continue;
+virReportSystemError(errno, %s,
+ _(error receiving from netlink
socket));
+rc = -1;
+goto err_exit;
+}
+rcvoffset += nbytes;
+break;
+}
+*respbuflen = rcvoffset;
+
+/* check message for error */
+if (*respbuflen NLMSG_LENGTH(0) *respbuf != NULL) {
+struct nlmsghdr *resp = (struct nlmsghdr *)*respbuf;
+struct nlmsgerr *err;
+
+
Re: [libvirt] [PATCH v3] Fix up basic migration.
Chris Lalancette wrote: Basic live migration was broken by the commit that added non-shared block support in two ways: 1) It added a virCheckFlags() to doNativeMigrate(). Besides ... diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 77e71cc..941b482 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10175,6 +10175,15 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, int ret = -1; int internalret; +virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, -1); Hi Chris, This looks like a fine change, but I haven't delved into it enough to give a formal ACK. However, one quick comment: It was not at all obvious to me that those three lists of eight VIR_* macros or'd together were identical. I used the editor to confirm it. Considering that someone might be tempted to modify one but miss the other two -- or add a 4th use, would it make sense to define a new symbol, and then use that in those three calls? #define VIR_MIGRATE_SOMETHING \ (VIR_MIGRATE_LIVE | \ VIR_MIGRATE_PEER2PEER | \ VIR_MIGRATE_TUNNELLED | \ VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE |\ VIR_MIGRATE_PAUSED | \ VIR_MIGRATE_NON_SHARED_DISK |\ VIR_MIGRATE_NON_SHARED_INC) ... +virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, -1); ... +virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, NULL); -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7] [REPOST] add 802.1Qbh and 802.1Qbg handling
Scott Feldman wrote:
On 5/25/10 10:24 AM, Stefan Berger stef...@linux.vnet.ibm.com wrote:
Reposting due to malformatted patch.
Thanks for fixing the malformed issue.
My testing is done with this version v7 plus the other patches list below.
No issues.
ACK.
[PATCH v10] vepa: parsing for 802.1Qb{g|h} XML
[RFC][PATCH 1/3] vepa+vsi: Introduce dependency on libnl
Since this one is already pushed,
[PATCH v3] Add host UUID (to libvirt capabilities)
I applied the other two, and then v7.
Once I fixed the unrelated make check failure,
I ran make check through valgrind and determined
that the definite leaks thus exposed are benign.
One bunch applies only to a test and the other is here:
80 bytes in 1 blocks are definitely lost in loss record 19 of 19
calloc (vg_replace_malloc.c:418)
virAlloc (memory.c:100)
virLastErrorObject (virterror.c:277)
virResetLastError (virterror.c:418)
vshDeinit (virsh.c:10285)
main (virsh.c:10482)
ACK.
For reference, I also computed the v6-v7 incremental, reviewed
it and attach it below:
diff --git a/configure.ac b/configure.ac
index 885b0ae..777dddc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2024,7 +2024,7 @@ dnl netlink library
LIBNL_CFLAGS=
LIBNL_LIBS=
-if test $with_macvtap = yes || $with_virtualport = yes; then
+if test $with_macvtap = yes || test $with_virtualport = yes; then
PKG_CHECK_MODULES([LIBNL], [libnl-1 = $LIBNL_REQUIRED], [
], [
AC_MSG_ERROR([libnl = $LIBNL_REQUIRED is required for macvtap
support])
diff --git a/src/util/macvtap.c b/src/util/macvtap.c
index 0224c65..ecaa1e6 100644
--- a/src/util/macvtap.c
+++ b/src/util/macvtap.c
@@ -63,6 +63,13 @@
# define MICROSEC_PER_SEC (1000 * 1000)
+# define NLMSGBUF_SIZE 256
+# define RATTBUF_SIZE 64
+
+
+# define STATUS_POLL_TIMEOUT_USEC (10 * MICROSEC_PER_SEC)
+# define STATUS_POLL_INTERVL_USEC (MICROSEC_PER_SEC / 8)
+
static int associatePortProfileId(const char *macvtap_ifname,
const char *linkdev,
@@ -108,7 +115,7 @@ static void nlClose(int fd)
*/
static
int nlComm(struct nlmsghdr *nlmsg,
- char **respbuf, int *respbuflen)
+ char **respbuf, unsigned int *respbuflen)
{
int rc = 0;
struct sockaddr_nl nladdr = {
@@ -180,8 +187,8 @@ err_exit:
* @respbuf: pointer to pointer where response buffer will be allocated
* @respbuflen: pointer to integer holding the size of the response buffer
* on return of the function.
- * @to_usecs: timeout in microseconds to wait for a success message
- *to be returned
+ * @timeout_usecs: timeout in microseconds to wait for a success message
+ * to be returned
*
* Send the given message to the netlink multicast group and receive
* responses. Skip responses indicating an error and keep on receiving
@@ -190,8 +197,9 @@ err_exit:
* buffer will be returned.
*/
static int
-nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
- char **respbuf, int *respbuflen, long to_usecs)
+nlCommWaitSuccess(struct nlmsghdr *nlmsg, uint32_t nl_groups,
+ char **respbuf, unsigned int *respbuflen,
+ unsigned long long timeout_usecs)
{
int rc = 0;
struct sockaddr_nl nladdr = {
@@ -200,15 +208,13 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
.nl_groups = nl_groups,
};
int rcvChunkSize = 1024; // expecting less than that
-int rcvoffset = 0;
+size_t rcv_offset = 0;
ssize_t nbytes;
-int n;
struct timeval tv = {
-.tv_sec = to_usecs / MICROSEC_PER_SEC,
-.tv_usec = to_usecs % MICROSEC_PER_SEC,
+.tv_sec = timeout_usecs / MICROSEC_PER_SEC,
+.tv_usec = timeout_usecs % MICROSEC_PER_SEC,
};
-fd_set rfds;
-bool gotvalid = false;
+bool got_valid = false;
int fd = nlOpen();
static uint32_t seq = 0x1234;
uint32_t myseq = seq++;
@@ -230,12 +236,16 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
goto err_exit;
}
-while (!gotvalid) {
-rcvoffset = 0;
+while (!got_valid) {
+
+rcv_offset = 0;
+
while (1) {
+int n;
+fd_set rfds;
socklen_t addrlen = sizeof(nladdr);
-if (VIR_REALLOC_N(*respbuf, rcvoffset+rcvChunkSize) 0) {
+if (VIR_REALLOC_N(*respbuf, rcv_offset + rcvChunkSize) 0) {
virReportOOMError();
rc = -1;
goto err_exit;
@@ -245,12 +255,18 @@ nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
FD_SET(fd, rfds);
n = select(fd + 1, rfds, NULL, NULL, tv);
-if (n == 0) {
+if (n = 0) {
+if (n 0)
+virReportSystemError(errno, %s,
+ _(error in select call));
+if (n == 0)
+virReportSystemError(ETIMEDOUT, %s,
Re: [libvirt] [PATCH 2/3] Docs for usage of new command APIs
Daniel P. Berrange wrote: --- docs/Makefile.am | 12 ++-- docs/internals.html.in |9 + docs/sitemap.html.in |4 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/docs/Makefile.am b/docs/Makefile.am ... + liGuide to adding a href=api_extension.htmlpublic APIsa/li + liApproach for a href=internals/command.htmlspawning commands/a from I don't see command.html.in anywhere. Did you mean to add it as part of this patch set? Avoiding this sort of problem is one of the reasons to prefer enumerating file names over using GNU make's $(wildcard function. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] avoid new make check failure and fix a related bug
In reviewing/testing some incoming patches, I noticed a new
make check failure.
There were two problems:
- not diagnosing an invalid host UUID. The lack of this diagnostic
made it slightly more challenging to track down the next problem:
- not accommodating the fact that our template now contains
an invalid line; before the addition of host_uuid, each
commented-out sample line was valid.
Here are the fixes.
From 11228f4b2dba1b43a9de97a575d1121eba2e8cf4 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 25 May 2010 20:56:18 +0200
Subject: [PATCH 1/2] libvirtd: diagnose invalid host UUID
* daemon/libvirtd.c (remoteReadConfigFile): Diagnose an invalid
host UUID rather than silently exiting with status 7.
---
daemon/libvirtd.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 8fa78b8..e86f78d 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -2843,8 +2843,10 @@ remoteReadConfigFile (struct qemud_server *server, const
char *filename)
GET_CONF_INT (conf, filename, max_client_requests);
GET_CONF_STR (conf, filename, host_uuid);
-if (virSetHostUUIDStr(host_uuid))
+if (virSetHostUUIDStr(host_uuid)) {
+VIR_ERROR(_(invalid host UUID: %s), host_uuid);
goto free_and_fail;
+}
VIR_FREE(host_uuid);
--
1.7.1.342.g1c280
From 6d20ae23fe5a5e64a0cf1be1f0b9cebce49625c1 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 25 May 2010 21:08:37 +0200
Subject: [PATCH 2/2] tests: avoid new failure of the daemon-conf test
* tests/daemon-conf: Accommodate the fact that out template,
daemon/libvirtd.conf now contains an invalid host_uuid.
Convert it to a valid one before the final libvirtd-running
test that must terminate normally.
---
tests/daemon-conf |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tests/daemon-conf b/tests/daemon-conf
index 14d4ced..0e756d4 100755
--- a/tests/daemon-conf
+++ b/tests/daemon-conf
@@ -83,7 +83,11 @@ if test 108 -lt `echo $SOCKPATH | wc -c`; then
skip_test_ CWD too long
fi
-$abs_top_builddir/daemon/libvirtd --pid-file=pid-file --config=tmp.conf log
21 pid=$!
+# Replace the invalid host_uuid with one that is valid:
+sed 's/^\(host_uuid =.*\)0$/\11/' tmp.conf k; mv k tmp.conf
+
+$abs_top_builddir/daemon/libvirtd --pid-file=pid-file --config=tmp.conf \
+ log 21 pid=$!
sleep $sleep_secs
kill $pid
--
1.7.1.342.g1c280
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7] add 802.1Qbh and 802.1Qbg handling
Stefan Berger wrote: This patch builds on the work recently posted by Stefan Berger. It builds on top of Stefan's three posted patches: Not a show-stopper, but your patch is corrupted due to split lines. If you tell your mail client not to do that, it'll be easier on folks who end up applying your patch. Also, we prefer it when contributors post git format-patch output or post via git send-email. The first split line happened with this added line: AC_DEFINE_UNQUOTED([WITH_VIRTUALPORT], $val, [whether vsi vepa support is enabled]) it's longer than 80, too, which we prefer to avoid. Better to write it like this: AC_DEFINE_UNQUOTED([WITH_VIRTUALPORT], $val, [whether vsi vepa support is enabled]) Hmm.. even after fixing two split lines, I'm getting lots of failed hunks and at least one more split line: $ patch -p1 j patching file configure.ac Hunk #1 FAILED at 2005. 1 out of 1 hunk FAILED -- saving rejects to file configure.ac.rej patching file src/qemu/qemu_conf.c Hunk #1 FAILED at 1509. 1 out of 1 hunk FAILED -- saving rejects to file src/qemu/qemu_conf.c.rej patching file src/qemu/qemu_driver.c Hunk #1 FAILED at 3709. Hunk #2 FAILED at 8514. 2 out of 2 hunks FAILED -- saving rejects to file src/qemu/qemu_driver.c.rej patching file src/util/macvtap.c Hunk #2 succeeded at 41 with fuzz 1. Hunk #3 succeeded at 49 (offset -1 lines). patch: malformed patch at line 210: virtPort, I'll try to straighten this out and repost shortly. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] VIR_ERROR and VIR_DEBUG normalization
Most of the following changes have been induced automatically. The few outliers done manually are marked as such. [PATCH 01/10] maint: use VIR_ERROR0 rather than VIR_ERROR with a bare %s [PATCH 02/10] maint: mark translatable string args of VIR_ERROR0 [PATCH 03/10] maint: mark translatable string args of VIR_ERROR [PATCH 04/10] maint: VIR_ERROR/VIR_ERROR0: mark up the remaining ones manually [PATCH 05/10] maint: more of same, but manual: convert VIR_ERROR(%s to VIR_ERROR0( [PATCH 06/10] maint: change in err ? err-message : to _(unknown error), ... [PATCH 07/10] maint: enforce policy wrt VIR_ERROR and VIR_ERROR0 [PATCH 08/10] maint: don't mark VIR_DEBUG or VIR_DEBUG0 diagnostics for translation [PATCH 09/10] maint: enforce policy wrt VIR_DEBUG and VIR_DEBUG0 [PATCH 10/10] maint: update po/POTFILES.in -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 01/10] maint: use VIR_ERROR0 rather than VIR_ERROR with a bare %s
From: Jim Meyering meyer...@redhat.com
Change VIR_ERROR(%s, ...
to VIR_ERROR0(...
and
Change VIR_ERROR(%s, _(...)
to VIR_ERROR0(_(...)
Use this command:
git grep -E -l 'VIR_ERROR\(%s, (_\()?'|xargs perl -pi -e \
's/VIR_ERROR\(%s, (_\()?/VIR_ERROR0($1/'
---
daemon/libvirtd.c | 12 ++--
src/phyp/phyp_driver.c | 16
src/qemu/qemu_conf.c |4 ++--
src/qemu/qemu_driver.c |2 +-
4 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index cc05953..0ac1f5e 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -539,7 +539,7 @@ static int qemudListenUnix(struct qemud_server *server,
char ebuf[1024];
if (VIR_ALLOC(sock) 0) {
-VIR_ERROR(%s, _(Failed to allocate memory for struct
qemud_socket));
+VIR_ERROR0(_(Failed to allocate memory for struct qemud_socket));
return -1;
}
@@ -844,12 +844,12 @@ static struct qemud_server *qemudInitialize(void) {
server-sigread = server-sigwrite = -1;
if (virMutexInit(server-lock) 0) {
-VIR_ERROR(%s, _(cannot initialize mutex));
+VIR_ERROR0(_(cannot initialize mutex));
VIR_FREE(server);
return NULL;
}
if (virCondInit(server-job) 0) {
-VIR_ERROR(%s, _(cannot initialize condition variable));
+VIR_ERROR0(_(cannot initialize condition variable));
virMutexDestroy(server-lock);
VIR_FREE(server);
return NULL;
@@ -1359,7 +1359,7 @@ static int qemudDispatchServer(struct qemud_server
*server, struct qemud_socket
if (VIR_ALLOC(client) 0)
goto cleanup;
if (virMutexInit(client-lock) 0) {
-VIR_ERROR(%s, _(cannot initialize mutex));
+VIR_ERROR0(_(cannot initialize mutex));
VIR_FREE(client);
goto cleanup;
}
@@ -2771,7 +2771,7 @@ remoteReadConfigFile (struct qemud_server *server, const
char *filename)
maxbuf = 1024;
if (VIR_ALLOC_N(buf, maxbuf) 0) {
-VIR_ERROR(%s, _(Failed to allocate memory for buffer));
+VIR_ERROR0(_(Failed to allocate memory for buffer));
goto free_and_fail;
}
@@ -2780,7 +2780,7 @@ remoteReadConfigFile (struct qemud_server *server, const
char *filename)
grp)) == ERANGE) {
maxbuf *= 2;
if (maxbuf 65536 || VIR_REALLOC_N(buf, maxbuf) 0) {
-VIR_ERROR(%s, _(Failed to reallocate enough memory
for buffer));
+VIR_ERROR0(_(Failed to reallocate enough memory for
buffer));
goto free_and_fail;
}
}
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 8f4f310..fbb094f 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1233,30 +1233,30 @@ phypDomainDumpXML(virDomainPtr dom, int flags)
dom-conn);
if (lpar_name == NULL) {
-VIR_ERROR(%s, Unable to determine domain's name.);
+VIR_ERROR0(Unable to determine domain's name.);
goto err;
}
if (phypGetLparUUID(def.uuid, dom-id, dom-conn) == -1) {
-VIR_ERROR(%s, Unable to generate random uuid.);
+VIR_ERROR0(Unable to generate random uuid.);
goto err;
}
if ((def.maxmem =
phypGetLparMem(dom-conn, managed_system, dom-id, 0)) == 0) {
-VIR_ERROR(%s, Unable to determine domain's max memory.);
+VIR_ERROR0(Unable to determine domain's max memory.);
goto err;
}
if ((def.memory =
phypGetLparMem(dom-conn, managed_system, dom-id, 1)) == 0) {
-VIR_ERROR(%s, Unable to determine domain's memory.);
+VIR_ERROR0(Unable to determine domain's memory.);
goto err;
}
if ((def.vcpus =
phypGetLparCPU(dom-conn, managed_system, dom-id)) == 0) {
-VIR_ERROR(%s, Unable to determine domain's CPU.);
+VIR_ERROR0(Unable to determine domain's CPU.);
goto err;
}
@@ -1695,7 +1695,7 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def)
}
if (phypUUIDTable_AddLpar(conn, def-uuid, def-id) == -1) {
-VIR_ERROR(%s, Unable to add LPAR to the table);
+VIR_ERROR0(Unable to add LPAR to the table);
goto err;
}
@@ -1835,13 +1835,13 @@ phypUUIDTable_WriteFile(virConnectPtr conn)
if (safewrite(fd, uuid_table-lpars[i]-id,
sizeof(uuid_table-lpars[i]-id)) !=
sizeof(uuid_table-lpars[i]-id)) {
-VIR_ERROR(%s, Unable to write information to local file.);
+VIR_ERROR0(Unable to write information to local file.);
goto err;
}
if (safewrite(fd, uuid_table-lpars[i]-uuid, VIR_UUID_BUFLEN) !=
VIR_UUID_BUFLEN) {
-VIR_ERROR(%s, Unable to write information to local file
[libvirt] [PATCH 02/10] maint: mark translatable string args of VIR_ERROR0
From: Jim Meyering meyer...@redhat.com
Run this:
git grep -l 'VIR_ERROR0\s*('|xargs perl -pi -e \
's/(VIR_ERROR0)\s*\((.*?)\)/$1(_($2))/'
---
daemon/libvirtd.c |6 +++---
src/esx/esx_driver.c |4 ++--
src/node_device/node_device_hal.c | 16
src/node_device/node_device_linux_sysfs.c |2 +-
src/node_device/node_device_udev.c|8
src/phyp/phyp_driver.c| 16
src/qemu/qemu_driver.c|4 ++--
src/uml/uml_driver.c |4 ++--
src/util/cgroup.c |4 ++--
src/xen/proxy_internal.c |2 +-
10 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 0ac1f5e..2f9c871 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -3199,7 +3199,7 @@ int main(int argc, char **argv) {
/* Start the event loop in a background thread, since
* state initialization needs events to be being processed */
if (qemudStartEventLoop(server) 0) {
-VIR_ERROR0(Event thread startup failed);
+VIR_ERROR0(_(Event thread startup failed));
goto error;
}
@@ -3208,14 +3208,14 @@ int main(int argc, char **argv) {
* we're ready, since it can take a long time and this will
* seriously delay OS bootup process */
if (virStateInitialize(server-privileged) 0) {
-VIR_ERROR0(Driver state initialization failed);
+VIR_ERROR0(_(Driver state initialization failed));
goto shutdown;
}
/* Start accepting new clients from network */
virMutexLock(server-lock);
if (qemudNetworkEnable(server) 0) {
-VIR_ERROR0(Network event loop enablement failed);
+VIR_ERROR0(_(Network event loop enablement failed));
goto shutdown;
}
virMutexUnlock(server-lock);
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 8e55fc6..7c9c50e 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -1892,14 +1892,14 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr
info)
esxVI_PerfEntityMetric_DynamicCast(perfEntityMetricBase);
if (perfMetricIntSeries == NULL) {
-VIR_ERROR0(QueryPerf returned object with unexpected type);
+VIR_ERROR0(_(QueryPerf returned object with unexpected
type));
}
perfMetricIntSeries =
esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric-value);
if (perfMetricIntSeries == NULL) {
-VIR_ERROR0(QueryPerf returned object with unexpected type);
+VIR_ERROR0(_(QueryPerf returned object with unexpected
type));
}
for (; perfMetricIntSeries != NULL;
diff --git a/src/node_device/node_device_hal.c
b/src/node_device/node_device_hal.c
index 8113c55..235bf56 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -718,22 +718,22 @@ static int halDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
dbus_error_init(err);
hal_ctx = libhal_ctx_new();
if (hal_ctx == NULL) {
-VIR_ERROR0(libhal_ctx_new returned NULL);
+VIR_ERROR0(_(libhal_ctx_new returned NULL));
goto failure;
}
dbus_conn = dbus_bus_get(DBUS_BUS_SYSTEM, err);
if (dbus_conn == NULL) {
-VIR_ERROR0(dbus_bus_get failed);
+VIR_ERROR0(_(dbus_bus_get failed));
goto failure;
}
dbus_connection_set_exit_on_disconnect(dbus_conn, FALSE);
if (!libhal_ctx_set_dbus_connection(hal_ctx, dbus_conn)) {
-VIR_ERROR0(libhal_ctx_set_dbus_connection failed);
+VIR_ERROR0(_(libhal_ctx_set_dbus_connection failed));
goto failure;
}
if (!libhal_ctx_init(hal_ctx, err)) {
-VIR_ERROR0(libhal_ctx_init failed, haldaemon is probably not
running);
+VIR_ERROR0(_(libhal_ctx_init failed, haldaemon is probably not
running));
/* We don't want to show a fatal error here,
otherwise entire libvirtd shuts down when
hald isn't running */
@@ -747,7 +747,7 @@ static int halDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
remove_dbus_watch,
toggle_dbus_watch,
NULL, NULL)) {
-VIR_ERROR0(dbus_connection_set_watch_functions failed);
+VIR_ERROR0(_(dbus_connection_set_watch_functions failed));
goto failure;
}
@@ -768,13 +768,13 @@ static int halDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
!libhal_ctx_set_device_lost_capability(hal_ctx, device_cap_lost) ||
!libhal_ctx_set_device_property_modified(hal_ctx,
device_prop_modified) ||
!libhal_device_property_watch_all(hal_ctx, err)) {
-VIR_ERROR0(setting up HAL callbacks
[libvirt] [PATCH 10/10] maint: update po/POTFILES.in
From: Jim Meyering meyer...@redhat.com * po/POTFILES.in: Add 3 files. --- po/POTFILES.in |7 +-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index e08b8c8..baaf56f 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -18,6 +18,7 @@ src/cpu/cpu_generic.c src/cpu/cpu_map.c src/cpu/cpu_x86.c src/datatypes.c +src/driver.c src/esx/esx_driver.c src/esx/esx_util.c src/esx/esx_vi.c @@ -32,6 +33,7 @@ src/lxc/lxc_controller.c src/lxc/lxc_driver.c src/network/bridge_driver.c src/node_device/node_device_driver.c +src/node_device/node_device_hal.c src/node_device/node_device_linux_sysfs.c src/node_device/node_device_udev.c src/nodeinfo.c @@ -53,8 +55,8 @@ src/qemu/qemu_monitor_text.c src/qemu/qemu_security_dac.c src/remote/remote_driver.c src/secret/secret_driver.c -src/security/security_driver.c src/security/security_apparmor.c +src/security/security_driver.c src/security/security_selinux.c src/security/virt-aa-helper.c src/storage/storage_backend.c @@ -70,6 +72,7 @@ src/uml/uml_conf.c src/uml/uml_driver.c src/util/authhelper.c src/util/bridge.c +src/util/cgroup.c src/util/conf.c src/util/dnsmasq.c src/util/hooks.c @@ -87,10 +90,10 @@ src/util/xml.c src/vbox/vbox_driver.c src/vbox/vbox_tmpl.c src/xen/proxy_internal.c -src/xen/xend_internal.c src/xen/xen_driver.c src/xen/xen_hypervisor.c src/xen/xen_inotify.c +src/xen/xend_internal.c src/xen/xm_internal.c src/xen/xs_internal.c src/xenapi/xenapi_driver.c -- 1.7.1.259.g3aef8 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 03/10] maint: mark translatable string args of VIR_ERROR
From: Jim Meyering meyer...@redhat.com
Run this:
git grep -l 'VIR_ERROR\s*('|xargs perl -pi -e \
's/(VIR_ERROR)\s*\((.*?),/$1(_($2),/'
---
daemon/libvirtd.c |2 +-
src/driver.c |6 +++---
src/libvirt.c |2 +-
src/lxc/lxc_controller.c |2 +-
src/node_device/node_device_hal.c |2 +-
src/node_device/node_device_linux_sysfs.c |8
src/node_device/node_device_udev.c| 20 ++--
src/phyp/phyp_driver.c| 10 +-
src/qemu/qemu_conf.c |2 +-
src/qemu/qemu_driver.c|8
src/qemu/qemu_monitor.c |4 ++--
src/storage/storage_driver.c |8
src/uml/uml_driver.c |4 ++--
src/util/cgroup.c | 10 +-
src/util/util.c |2 +-
15 files changed, 45 insertions(+), 45 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 2f9c871..04af58f 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -817,7 +817,7 @@ static int qemudInitPaths(struct qemud_server *server,
snprintf_error:
if (ret)
-VIR_ERROR(%s,
+VIR_ERROR(_(%s),
_(Resulting path too long for buffer in qemudInitPaths()));
cleanup:
diff --git a/src/driver.c b/src/driver.c
index e6f3aaa..a6f5558 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -64,7 +64,7 @@ virDriverLoadModule(const char *name)
handle = dlopen(modfile, RTLD_NOW | RTLD_LOCAL);
if (!handle) {
-VIR_ERROR(failed to load module %s %s, modfile, dlerror());
+VIR_ERROR(_(failed to load module %s %s), modfile, dlerror());
goto cleanup;
}
@@ -74,12 +74,12 @@ virDriverLoadModule(const char *name)
regsym = dlsym(handle, regfunc);
if (!regsym) {
-VIR_ERROR(Missing module registration symbol %s, regfunc);
+VIR_ERROR(_(Missing module registration symbol %s), regfunc);
goto cleanup;
}
if ((*regsym)() 0) {
-VIR_ERROR(Failed module registration %s, regfunc);
+VIR_ERROR(_(Failed module registration %s), regfunc);
goto cleanup;
}
diff --git a/src/libvirt.c b/src/libvirt.c
index eb05337..9d42c76 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -976,7 +976,7 @@ int virStateInitialize(int privileged) {
for (i = 0 ; i virStateDriverTabCount ; i++) {
if (virStateDriverTab[i]-initialize
virStateDriverTab[i]-initialize(privileged) 0) {
-VIR_ERROR(Initialization of %s state driver failed,
+VIR_ERROR(_(Initialization of %s state driver failed),
virStateDriverTab[i]-name);
ret = -1;
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 1732780..6b64372 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -302,7 +302,7 @@ static int lxcControllerMain(int monitor,
fdArray[0].active = 0;
fdArray[1].fd = contPty;
fdArray[1].active = 0;
-VIR_ERROR(monitor=%d client=%d appPty=%d contPty=%d, monitor,client,
appPty, contPty);
+VIR_ERROR(_(monitor=%d client=%d appPty=%d contPty=%d), monitor,client,
appPty, contPty);
/* create the epoll fild descriptor */
epollFd = epoll_create(2);
if (0 epollFd) {
diff --git a/src/node_device/node_device_hal.c
b/src/node_device/node_device_hal.c
index 235bf56..c2b3c8f 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -787,7 +787,7 @@ static int halDeviceMonitorStartup(int privileged
ATTRIBUTE_UNUSED)
failure:
if (dbus_error_is_set(err)) {
-VIR_ERROR(%s: %s, err.name, err.message);
+VIR_ERROR(_(%s: %s), err.name, err.message);
dbus_error_free(err);
}
virNodeDeviceObjListFree(driverState-devs);
diff --git a/src/node_device/node_device_linux_sysfs.c
b/src/node_device/node_device_linux_sysfs.c
index 3e0c8cf..142b882 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -197,7 +197,7 @@ static int logStrToLong_ui(char const *s,
ret = virStrToLong_ui(s, end_ptr, base, result);
if (ret != 0) {
-VIR_ERROR(Failed to convert '%s' to unsigned int, s);
+VIR_ERROR(_(Failed to convert '%s' to unsigned int), s);
} else {
VIR_DEBUG(Converted '%s' to unsigned int %u, s, *result);
}
@@ -264,7 +264,7 @@ static int get_sriov_function(const char *device_link,
device_path = canonicalize_file_name (device_link);
if (device_path == NULL) {
memset(errbuf, '\0', sizeof(errbuf));
-VIR_ERROR(Failed to resolve device link '%s': '%s', device_link,
+VIR_ERROR(_(Failed to resolve device link '%s': '%s'), device_link,
virStrerror(errno, errbuf, sizeof(errbuf
[libvirt] [PATCH 05/10] maint: more of same, but manual: convert VIR_ERROR(%s to VIR_ERROR0(
From: Jim Meyering meyer...@redhat.com
---
daemon/libvirtd.c |3 +--
src/phyp/phyp_driver.c | 12 +---
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 04af58f..afa10fb 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -817,8 +817,7 @@ static int qemudInitPaths(struct qemud_server *server,
snprintf_error:
if (ret)
-VIR_ERROR(_(%s),
- _(Resulting path too long for buffer in qemudInitPaths()));
+VIR_ERROR0(_(Resulting path too long for buffer in
qemudInitPaths()));
cleanup:
VIR_FREE(dir_prefix);
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 4a10461..8a9c7a6 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1520,9 +1520,8 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
goto err;
if (nvcpus phypGetLparCPUMAX(dom)) {
-VIR_ERROR(_(%s),
- You are trying to set a number of CPUs bigger than
- the max possible..);
+VIR_ERROR0(_(You are trying to set a number of CPUs bigger than
+ the max possible..));
goto err;
}
@@ -1547,9 +1546,8 @@ phypDomainSetCPU(virDomainPtr dom, unsigned int nvcpus)
ret = phypExec(session, cmd, exit_status, dom-conn);
if (exit_status 0) {
-VIR_ERROR(_(%s),
- Possibly you don't have IBM Tools installed in your LPAR.
- Contact your support to enable this feature.);
+VIR_ERROR0(_(Possibly you don't have IBM Tools installed in your
LPAR.
+ Contact your support to enable this feature.));
goto err;
}
@@ -1690,7 +1688,7 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def)
ret = phypExec(session, cmd, exit_status, conn);
if (exit_status 0) {
-VIR_ERROR(_(%s\%s\), Unable to create LPAR. Reason: , ret);
+VIR_ERROR(_(Unable to create LPAR. Reason: '%s'), ret);
goto err;
}
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/10] maint: change in err ? err-message : to _(unknown error), ...
From: Jim Meyering meyer...@redhat.com
These changes avoid false-positive syntax-check failure,
and also make the resulting diagnostics more comprehensible.
---
src/qemu/qemu_driver.c |8
src/secret/secret_driver.c |4 ++--
src/uml/uml_driver.c |2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0f9acb2..97991d7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -634,7 +634,7 @@ qemuAutostartDomain(void *payload, const char *name
ATTRIBUTE_UNUSED, void *opaq
virErrorPtr err = virGetLastError();
VIR_ERROR(_(Failed to autostart VM '%s': %s),
vm-def-name,
- err ? err-message : );
+ err ? err-message : _(unknown error));
} else {
virDomainEventPtr event =
virDomainEventNewFromObj(vm,
@@ -2895,7 +2895,7 @@ qemudReattachManagedDevice(pciDevice *dev)
if (pciReAttachDevice(dev) 0) {
virErrorPtr err = virGetLastError();
VIR_ERROR(_(Failed to re-attach PCI device: %s),
- err ? err-message : );
+ err ? err-message : _(unknown error));
virResetError(err);
}
}
@@ -2914,7 +2914,7 @@ qemuDomainReAttachHostDevices(struct qemud_driver *driver,
if (!(pcidevs = qemuGetPciHostDeviceList(def))) {
virErrorPtr err = virGetLastError();
VIR_ERROR(_(Failed to allocate pciDeviceList: %s),
- err ? err-message : );
+ err ? err-message : _(unknown error));
virResetError(err);
return;
}
@@ -2932,7 +2932,7 @@ qemuDomainReAttachHostDevices(struct qemud_driver *driver,
if (pciResetDevice(dev, driver-activePciHostdevs) 0) {
virErrorPtr err = virGetLastError();
VIR_ERROR(_(Failed to reset PCI device: %s),
- err ? err-message : );
+ err ? err-message : _(unknown error));
virResetError(err);
}
}
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 22852a1..01c0034 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -1,7 +1,7 @@
/*
* secret_driver.c: local driver for secret manipulation API
*
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 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
@@ -499,7 +499,7 @@ loadSecrets(virSecretDriverStatePtr driver,
virErrorPtr err = virGetLastError();
VIR_ERROR(_(Error reading secret: %s),
- err != NULL ? err-message: );
+ err != NULL ? err-message: _(unknown error));
virResetError(err);
continue;
}
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index da8fd47..c6d8b65 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -157,7 +157,7 @@ umlAutostartDomain(void *payload, const char *name
ATTRIBUTE_UNUSED, void *opaqu
if (umlStartVMDaemon(data-conn, data-driver, vm) 0) {
virErrorPtr err = virGetLastError();
VIR_ERROR(_(Failed to autostart VM '%s': %s),
- vm-def-name, err ? err-message : );
+ vm-def-name, err ? err-message : _(unknown error));
}
}
virDomainObjUnlock(vm);
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 04/10] maint: VIR_ERROR/VIR_ERROR0: mark up the remaining ones manually
From: Jim Meyering meyer...@redhat.com
Handle concatenated strings manually.
---
src/esx/esx_vi.c | 10 +-
src/node_device/node_device_udev.c | 20 ++--
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 966ef85..4d175e9 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -2770,13 +2770,13 @@ esxVI_WaitForTaskCompletion(esxVI_Context *ctx,
if (taskInfo-cancelable == esxVI_Boolean_True) {
if (esxVI_CancelTask(ctx, task) 0) {
-VIR_ERROR0(Cancelable task is blocked by an
- unanswered question but cancelation
- failed);
+VIR_ERROR0(_(Cancelable task is blocked by an
+ unanswered question but cancelation
+ failed));
}
} else {
-VIR_ERROR0(Non-cancelable task is blocked by an
- unanswered question);
+VIR_ERROR0(_(Non-cancelable task is blocked by an
+ unanswered question));
}
/* FIXME: Enable error reporting here again */
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index ee90537..a1ced87 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -122,8 +122,8 @@ static int udevGetDeviceProperty(struct udev_device
*udev_device,
* of the function must also be changed. */
*property_value = strdup(udev_value);
if (*property_value == NULL) {
-VIR_ERROR(Failed to allocate memory for property value for
- property key '%s' on device with sysname '%s',
+VIR_ERROR(_(Failed to allocate memory for property value for
+property key '%s' on device with sysname '%s'),
property_key, udev_device_get_sysname(udev_device));
virReportOOMError();
ret = PROPERTY_ERROR;
@@ -211,8 +211,8 @@ static int udevGetDeviceSysfsAttr(struct udev_device
*udev_device,
* of the function must also be changed. */
*attr_value = strdup(udev_value);
if (*attr_value == NULL) {
-VIR_ERROR(Failed to allocate memory for sysfs attribute value for
- sysfs attribute '%s' on device with sysname '%s',
+VIR_ERROR(_(Failed to allocate memory for sysfs attribute value for
+sysfs attribute '%s' on device with sysname '%s'),
attr_name, udev_device_get_sysname(udev_device));
virReportOOMError();
ret = PROPERTY_ERROR;
@@ -329,8 +329,8 @@ static int udevGenerateDeviceName(struct udev_device
*device,
if (virBufferError(buf)) {
virBufferFreeAndReset(buf);
-VIR_ERROR(Buffer error when generating device name for device
- with sysname '%s', udev_device_get_sysname(device));
+VIR_ERROR(_(Buffer error when generating device name for device
+with sysname '%s'), udev_device_get_sysname(device));
ret = -1;
}
@@ -639,8 +639,8 @@ static int udevProcessSCSIHost(struct udev_device *device
ATTRIBUTE_UNUSED,
filename = basename(def-sysfs_path);
if (!STRPREFIX(filename, host)) {
-VIR_ERROR(SCSI host found, but its udev name '%s' does
- not begin with 'host', filename);
+VIR_ERROR(_(SCSI host found, but its udev name '%s' does
+not begin with 'host'), filename);
goto out;
}
@@ -1401,8 +1401,8 @@ static void udevEventHandleCallback(int watch
ATTRIBUTE_UNUSED,
udev_fd = udev_monitor_get_fd(udev_monitor);
if (fd != udev_fd) {
-VIR_ERROR(File descriptor returned by udev %d does not
- match node device file descriptor %d, fd, udev_fd);
+VIR_ERROR(_(File descriptor returned by udev %d does not
+match node device file descriptor %d), fd, udev_fd);
goto out;
}
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 09/10] maint: enforce policy wrt VIR_DEBUG and VIR_DEBUG0
From: Jim Meyering meyer...@redhat.com
* cfg.mk (sc_prohibit_gettext_markup): Just like VIR_WARN*.
---
cfg.mk |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index a26285f..2909e3e 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -425,7 +425,7 @@ sc_copyright_format:
# Some functions/macros produce messages intended solely for developers
# and maintainers. Do not mark them for translation.
sc_prohibit_gettext_markup:
- @prohibit='\VIR_WARN0? *\(_\(' \
+ @prohibit='\VIR_(WARN|DEBUG)0? *\(_\(' \
halt='do not mark these strings for translation'\
$(_sc_search_regexp)
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 08/10] maint: don't mark VIR_DEBUG or VIR_DEBUG0 diagnostics for translation
From: Jim Meyering meyer...@redhat.com
Run this command:
git grep -l VIR_DEBUG|xargs perl -pi -e \
's/(VIR_DEBUG0?)\s*\(_\((.*?)\)/$1($2/'
---
...-Step-7-of-8-Implement-the-driver-methods.patch |4 +-
src/node_device/node_device_driver.c |2 +-
src/node_device/node_device_linux_sysfs.c |6 ++--
src/qemu/qemu_conf.c |2 +-
src/storage/storage_backend_scsi.c | 38 ++--
5 files changed, 26 insertions(+), 26 deletions(-)
diff --git
a/docs/api_extension/0007-Step-7-of-8-Implement-the-driver-methods.patch
b/docs/api_extension/0007-Step-7-of-8-Implement-the-driver-methods.patch
index 807ed78..ff1124f 100644
--- a/docs/api_extension/0007-Step-7-of-8-Implement-the-driver-methods.patch
+++ b/docs/api_extension/0007-Step-7-of-8-Implement-the-driver-methods.patch
@@ -146,7 +146,7 @@ index b84729f..4f73baf 100644
+goto cleanup;
+}
+
-+VIR_DEBUG(_(Vport operation path is '%s'), operation_path);
++VIR_DEBUG(Vport operation path is '%s', operation_path);
+
+fd = open(operation_path, O_WRONLY);
+
@@ -959,7 +959,7 @@ index 000..1deb6d2
+char buf[64];
+struct stat st;
+
-+VIR_DEBUG(_(Checking if host%d is an FC HBA), d-scsi_host.host);
++VIR_DEBUG(Checking if host%d is an FC HBA, d-scsi_host.host);
+
+if (virAsprintf(sysfs_path, %s/host%d,
+LINUX_SYSFS_FC_HOST_PREFIX,
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index b0ea986..a6c1fa0 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -432,7 +432,7 @@ nodeDeviceVportCreateDelete(const int parent_host,
goto cleanup;
}
-VIR_DEBUG(_(Vport operation path is '%s'), operation_path);
+VIR_DEBUG(Vport operation path is '%s', operation_path);
if (virAsprintf(vport_name,
%s:%s,
diff --git a/src/node_device/node_device_linux_sysfs.c
b/src/node_device/node_device_linux_sysfs.c
index 142b882..c90e72b 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -53,7 +53,7 @@ static int open_wwn_file(const char *prefix,
/* fd will be closed by caller */
if ((*fd = open(wwn_path, O_RDONLY)) != -1) {
-VIR_DEBUG(_(Opened WWN path '%s' for reading),
+VIR_DEBUG(Opened WWN path '%s' for reading,
wwn_path);
} else {
VIR_ERROR(_(Failed to open WWN path '%s' for reading),
@@ -79,7 +79,7 @@ int read_wwn_linux(int host, const char *file, char **wwn)
memset(buf, 0, sizeof(buf));
if (saferead(fd, buf, sizeof(buf)) 0) {
retval = -1;
-VIR_DEBUG(_(Failed to read WWN for host%d '%s'),
+VIR_DEBUG(Failed to read WWN for host%d '%s',
host, file);
goto out;
}
@@ -117,7 +117,7 @@ int check_fc_host_linux(union _virNodeDevCapData *d)
int retval = 0;
struct stat st;
-VIR_DEBUG(_(Checking if host%d is an FC HBA), d-scsi_host.host);
+VIR_DEBUG(Checking if host%d is an FC HBA, d-scsi_host.host);
if (virAsprintf(sysfs_path, %s/host%d,
LINUX_SYSFS_FC_HOST_PREFIX,
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index ee24c4c..5c14eb8 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -727,7 +727,7 @@ qemudProbeCPUModels(const char *qemu,
if (STREQ(arch, i686) || STREQ(arch, x86_64))
parse = qemudParseX86Models;
else {
-VIR_DEBUG(_(don't know how to parse %s CPU models), arch);
+VIR_DEBUG(don't know how to parse %s CPU models, arch);
return 0;
}
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index 0260818..cd01f93 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -92,7 +92,7 @@ getDeviceType(uint32_t host,
goto out;
}
-VIR_DEBUG(_(Device type is %d), *type);
+VIR_DEBUG(Device type is %d, *type);
out:
VIR_FREE(type_path);
@@ -221,7 +221,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
goto free_vol;
}
-VIR_DEBUG(_(Trying to create volume for '%s'), devpath);
+VIR_DEBUG(Trying to create volume for '%s', devpath);
/* Now figure out the stable path
*
@@ -239,7 +239,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
!(STREQ(pool-def-target.path, /dev) ||
STREQ(pool-def-target.path, /dev/))) {
-VIR_DEBUG(_(No stable path found for '%s' in '%s'),
+VIR_DEBUG(No stable path found for '%s' in '%s',
devpath, pool-def-target.path);
retval = -1;
@@ -301,7 +301,7 @@ getNewStyleBlockDevice(const char *lun_path,
goto out;
}
-VIR_DEBUG(_(Looking for block device in '%s'), block_path);
+VIR_DEBUG(Looking for block device in '%s', block_path);
block_dir = opendir
[libvirt] [PATCH 07/10] maint: enforce policy wrt VIR_ERROR and VIR_ERROR0
From: Jim Meyering meyer...@redhat.com * cfg.mk (msg_gen_function): Add VIR_ERROR and VIR_ERROR0, so that sc_libvirt_unmarked_diagnostics will check them, too. --- cfg.mk |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/cfg.mk b/cfg.mk index 04719d4..a26285f 100644 --- a/cfg.mk +++ b/cfg.mk @@ -358,6 +358,8 @@ msg_gen_function += virXendError msg_gen_function += xenapiSessionErrorHandler msg_gen_function += xenUnifiedError msg_gen_function += xenXMError +msg_gen_function += VIR_ERROR +msg_gen_function += VIR_ERROR0 # Uncomment the following and run make syntax-check to see diagnostics # that are not yet marked for translation, but that need to be rewritten -- 1.7.1.259.g3aef8 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libvirtd: mark strings for translation, including --help output
Saw these while fixing the previous bug:
From ab3d0ba4f5c004a45da5b5a240a0b5caef21df99 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 10:01:32 +0200
Subject: [PATCH] libvirtd: mark strings for translation, including --help output
* daemon/libvirtd.c (daemonForkIntoBackground, main): Mark strings
for translation.
(usage): Rework --help so that it is translatable, replacing
each embedded, configuration-dependent, macro with an `%s'.
libvirtd: don't ignore virInitialize failure
* daemon/libvirtd.c (main): Diagnose virInitialize failure
and exit nonzero.
---
daemon/libvirtd.c | 36 +---
1 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index aac2d08..d52b6eb 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -484,8 +484,8 @@ static int daemonForkIntoBackground(void) {
if (ret == 1 status != 0) {
fprintf(stderr,
-error: %s. Check /var/log/messages or run without
---daemon for more info.\n,
+_(error: %s. Check /var/log/messages or run without
+ --daemon for more info.\n),
virDaemonErrTypeToString(status));
}
_exit(ret == 1 status == 0 ? 0 : 1);
@@ -2963,7 +2963,7 @@ static void
usage (const char *argv0)
{
fprintf (stderr,
- \n\
+ _(\n\
Usage:\n\
%s [options]\n\
\n\
@@ -2981,27 +2981,33 @@ libvirt management daemon:\n\
Default paths:\n\
\n\
Configuration file (unless overridden by -f):\n\
- SYSCONF_DIR /libvirt/libvirtd.conf\n\
+ %s/libvirt/libvirtd.conf\n\
\n\
Sockets (as root):\n\
- LOCAL_STATE_DIR /run/libvirt/libvirt-sock\n\
- LOCAL_STATE_DIR /run/libvirt/libvirt-sock-ro\n\
+ %s/run/libvirt/libvirt-sock\n\
+ %s/run/libvirt/libvirt-sock-ro\n\
\n\
Sockets (as non-root):\n\
$HOME/.libvirt/libvirt-sock (in UNIX abstract namespace)\n\
\n\
TLS:\n\
- CA certificate: LIBVIRT_CACERT \n\
- Server certificate: LIBVIRT_SERVERCERT \n\
- Server private key: LIBVIRT_SERVERKEY \n\
+ CA certificate: %s\n\
+ Server certificate: %s\n\
+ Server private key: %s\n\
\n\
PID file (unless overridden by --pid-file):\n\
%s\n\
-\n,
- argv0,
- REMOTE_PID_FILE[0] != '\0'
- ? REMOTE_PID_FILE
- : (disabled in ./configure));
+\n),
+ argv0,
+ SYSCONF_DIR,
+ LOCAL_STATE_DIR,
+ LOCAL_STATE_DIR,
+ LIBVIRT_CACERT,
+ LIBVIRT_SERVERCERT,
+ LIBVIRT_SERVERKEY,
+ (REMOTE_PID_FILE[0] != '\0'
+? REMOTE_PID_FILE
+: (disabled in ./configure)));
}
enum {
@@ -3083,7 +3089,7 @@ int main(int argc, char **argv) {
return 2;
default:
-fprintf (stderr, libvirtd: internal error: unknown flag: %c\n,
+fprintf (stderr, _(libvirtd: internal error: unknown flag: %c\n),
c);
exit (EXIT_FAILURE);
}
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libvirtd: don't ignore virInitialize failure
I almost missed this; it was so similar to what I though
was the sole ignore-virInitialize-failure bug (in python set-up)
that I thought I'd already fixed it.
From f48e364b6f2182f7fa88862b2e1a789e2d83a027 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 09:52:20 +0200
Subject: [PATCH] libvirtd: don't ignore virInitialize failure
* daemon/libvirtd.c (main): Diagnose virInitialize failure
and exit nonzero.
---
daemon/libvirtd.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index afa10fb..aac2d08 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -3028,7 +3028,10 @@ int main(int argc, char **argv) {
{0, 0, 0, 0}
};
-virInitialize();
+if (virInitialize() 0) {
+fprintf (stderr, _(libvirtd: initialization failed\n));
+exit (EXIT_FAILURE);
+}
while (1) {
int optidx = 0;
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] maint: avoid unwanted newline at end of diagnostic
While I was in the virtual vicinity,
Jiri mentioned the possibility of detecting one more minor
formatting problem: the stray newline at the end of a diagnostic.
Here are two corrections and the heuristic that caught them
(and that will prevent introduction of some new ones -- but not if the
end of the message is more than 2 lines after the function name).
Currently there are no false positives.
From 122b1e31fb33c092a53802b56a0f2f5586c95bd5 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 11:12:17 +0200
Subject: [PATCH 1/2] maint: remove unwanted newline at end of diagnostic
* src/xen/xend_internal.c (xenDaemonDomainDefineXML): Remove \n.
* src/network/bridge_driver.c (networkAddMasqueradingIptablesRules):
Likewise.
---
src/network/bridge_driver.c |2 +-
src/xen/xend_internal.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3b9b4f4..5d7ef19 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -642,7 +642,7 @@ networkAddMasqueradingIptablesRules(struct network_driver
*driver,
network-def-network,
network-def-forwardDev))) {
virReportSystemError(err,
- _(failed to add iptables rule to enable
masquerading to '%s'\n),
+ _(failed to add iptables rule to enable
masquerading to '%s'),
network-def-forwardDev ?
network-def-forwardDev : NULL);
goto masqerr3;
}
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index ea5addd..a203a8d 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -4731,7 +4731,7 @@ virDomainPtr xenDaemonDomainDefineXML(virConnectPtr conn,
const char *xmlDesc) {
VIR_FREE(sexpr);
if (ret != 0) {
virXendError(VIR_ERR_XEN_CALL,
- _(Failed to create inactive domain %s\n), def-name);
+ _(Failed to create inactive domain %s), def-name);
goto error;
}
--
1.7.1.259.g3aef8
From 532672f3a0c53213609dab49df2e7efcaf0eb594 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 11:13:41 +0200
Subject: [PATCH 2/2] maint: prohibit newline at end of diagnostic
* cfg.mk (sc_prohibit_newline_at_end_of_diagnostic): New rule.
Idea proposed by Jiri Denemark.
---
cfg.mk | 14 ++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 2909e3e..b024c75 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -391,6 +391,20 @@ sc_libvirt_unmarked_diagnostics:
{ echo '$(ME): found unmarked diagnostic(s)' 12;\
exit 1; } || :
+# Like the above, but prohibit a newline at the end of a diagnostic.
+# This is subject to false positives partly because it naively looks for
+# `\n', which may not be the end of the string, and also because it takes
+# two lines of context (the -A2) after the line with the function name.
+# FIXME: this rule might benefit from a separate function list, in case
+# there are functions to which this one applies but that do not get marked
+# diagnostics.
+sc_prohibit_newline_at_end_of_diagnostic:
+ @grep -A2 -nE \
+ '\$(func_re) *\(' $$($(VC_LIST_EXCEPT))\
+ | grep '\\n' \
+ { echo '$(ME): newline at end of message(s)' 12; \
+ exit 1; } || :
+
# Disallow trailing blank lines.
sc_prohibit_trailing_blank_lines:
@$(VC_LIST_EXCEPT) | xargs perl -ln -0777 -e\
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] lxc_controller: don't ignore failed accept
Initially I proposed a similar patch here:
http://thread.gmane.org/gmane.comp.emulators.libvirt/20607
That thread languished, and Eric proposed a nearly identical patch:
http://thread.gmane.org/gmane.comp.emulators.libvirt/21630
Here's a patch that should address the initial objection.
Now, my only question is about which errno values to ignore.
Obviously, EMFILE, EFAULT, etc. must not be ignored.
I doubt EINTR and EBADF should be ignored, but haven't
tried to prove the case.
From 6dcd15b0aa594f3d89e810e0a92aa75dab22903e Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 14:30:36 +0200
Subject: [PATCH] lxc_controller.c: don't ignore failed accept
* src/lxc/lxc_controller.c (ignorable_epoll_accept_errno): New function.
(lxcControllerMain): Handle a failed accept carefully:
most errno values indicate legitimate failure and must be fatal.
However, ignore a special case: that in which an incoming client quits
between the poll() indicating its presence, and our accept() which
is trying to process it.
---
src/lxc/lxc_controller.c | 23 +++
1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 6b64372..75a45e9 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -269,6 +269,17 @@ typedef struct _lxcTtyForwardFd_t {
int active;
} lxcTtyForwardFd_t;
+/* Return true if it is ok to ignore an accept-after-epoll syscall
+ that fails with the specified errno value. Else false. */
+static bool
+ignorable_epoll_accept_errno(int erratum)
+{
+ return (errnum == EINVAL
+ || errnum == ECONNABORTED
+ || errnum == EAGAIN
+ || errnum == EWOULDBLOCK);
+}
+
/**
* lxcControllerMain
* @monitor: server socket fd to accept client requests
@@ -350,6 +361,18 @@ static int lxcControllerMain(int monitor,
if (numEvents 0) {
if (epollEvent.data.fd == monitor) {
int fd = accept(monitor, NULL, 0);
+if (fd 0) {
+/* First reflex may be simply to declare accept failure
+ to be a fatal error. However, accept may fail when
+ a client quits between the above epoll_wait and here.
+ That case is not fatal, but rather to be expected,
+ if not common, so ignore it. */
+if (ignorable_epoll_accept_errno(errno))
+continue;
+virReportSystemError(errno, %s,
+ _(accept(monitor,...) failed));
+goto cleanup;
+}
if (client != -1) { /* Already connected, so kick new one out
*/
close(fd);
continue;
--
1.7.1.262.g5ef3d
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu driver: fix version parsing fix
Chris Wright wrote: * Eric Blake (ebl...@redhat.com) wrote: On 05/19/2010 04:37 PM, Chris Wright wrote: Looks like some cut'n paste error to me. While we're at it, there have been some complaints, at least on IRC, that some recent qemu builds changed -help output to start with QEMU emulator version instead of QEMU PC emulator version, which fails to match QEMU_VERSION_STR. Is that something we should be worrying about, while we're touching this function? That's what had me looking at it ;) [chr...@x200 qemu-kvm]$ ./x86_64-softmmu/qemu-system-x86_64 -help | head -1 QEMU emulator version 0.12.50 (qemu-kvm-devel), Copyright (c) 2003-2008 Fabrice Bellard [chr...@x200 qemu-kvm]$ qemu-kvm -help | head -1 QEMU PC emulator version 0.11.0 (qemu-kvm-0.11.0), Copyright (c) 2003-2008 Fabrice Bellard This keys off of only emulator version, so should not have the same parsing issue. Signed-off-by: Chris Wright chr...@redhat.com --- src/qemu/qemu_conf.c |9 ++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5fa8c0a..359c311 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1247,6 +1247,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, /* We parse the output of 'qemu -help' to get the QEMU * version number. The first bit is easy, just parse * 'QEMU PC emulator version x.y.z'. + * or + * 'QEMU emulator version x.y.z'. * * With qemu-kvm, however, that is followed by a string * in parenthesis as follows: @@ -1259,7 +1261,7 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STRQEMU PC emulator version +#define QEMU_VERSION_STRemulator version #define QEMU_KVM_VER_PREFIX (qemu-kvm- #define KVM_VER_PREFIX (kvm- @@ -1277,9 +1279,10 @@ int qemudParseHelpStr(const char *qemu, *flags = *version = *is_kvm = *kvm_version = 0; -if (!STRPREFIX(p, QEMU_VERSION_STR)) +p = strstr(p, QEMU_VERSION_STR); +if (!p) goto fail; p += strlen(QEMU_VERSION_STR); Hi Chris, That patch looks fine, and is nicely minimal. However, it does relax the test significantly. Rather than requiring that QEMU_VERSION_STR be a prefix, it would allow it to appear anywhere within the -help output. While I really doubt it'd ever make a difference, it's easy to retain the stricter test, so I wrote this. Either way is fine with me. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5c14eb8..f43c6e2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1259,7 +1259,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STRQEMU PC emulator version +#define QEMU_VERSION_STR_1 QEMU PC emulator version +#define QEMU_VERSION_STR_2 QEMU emulator version #define QEMU_KVM_VER_PREFIX (qemu-kvm- #define KVM_VER_PREFIX (kvm- @@ -1277,11 +1278,13 @@ int qemudParseHelpStr(const char *qemu, *flags = *version = *is_kvm = *kvm_version = 0; -if (!STRPREFIX(p, QEMU_VERSION_STR)) +if (STRPREFIX(p, QEMU_VERSION_STR_1)) +p += strlen(QEMU_VERSION_STR_1); +else if (STRPREFIX(p, QEMU_VERSION_STR_2)) +p += strlen(QEMU_VERSION_STR_2); +else goto fail; -p += strlen(QEMU_VERSION_STR); - SKIP_BLANKS(p); major = virParseNumber(p); -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] lxc_controller: don't ignore failed accept
Jim Meyering wrote:
Initially I proposed a similar patch here:
http://thread.gmane.org/gmane.comp.emulators.libvirt/20607
That thread languished, and Eric proposed a nearly identical patch:
http://thread.gmane.org/gmane.comp.emulators.libvirt/21630
Here's a patch that should address the initial objection.
Now, my only question is about which errno values to ignore.
Obviously, EMFILE, EFAULT, etc. must not be ignored.
I doubt EINTR and EBADF should be ignored, but haven't
tried to prove the case.
From 6dcd15b0aa594f3d89e810e0a92aa75dab22903e Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 14:30:36 +0200
Subject: [PATCH] lxc_controller.c: don't ignore failed accept
...
+static bool
+ignorable_epoll_accept_errno(int erratum)
That didn't even compile.
Here's the same patch modulo s/erratum/errnum/,
From d4bebc04bf701e81efdd9d68ceb5b179a625e1db Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 14:30:36 +0200
Subject: [PATCH] lxc_controller.c: don't ignore failed accept
* src/lxc/lxc_controller.c (ignorable_epoll_accept_errno): New function.
(lxcControllerMain): Handle a failed accept carefully:
most errno values indicate legitimate failure and must be fatal.
However, ignore a special case: that in which an incoming client quits
between the poll() indicating its presence, and our accept() which
is trying to process it.
---
src/lxc/lxc_controller.c | 23 +++
1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 6b64372..cffef52 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -269,6 +269,17 @@ typedef struct _lxcTtyForwardFd_t {
int active;
} lxcTtyForwardFd_t;
+/* Return true if it is ok to ignore an accept-after-epoll syscall
+ that fails with the specified errno value. Else false. */
+static bool
+ignorable_epoll_accept_errno(int errnum)
+{
+ return (errnum == EINVAL
+ || errnum == ECONNABORTED
+ || errnum == EAGAIN
+ || errnum == EWOULDBLOCK);
+}
+
/**
* lxcControllerMain
* @monitor: server socket fd to accept client requests
@@ -350,6 +361,18 @@ static int lxcControllerMain(int monitor,
if (numEvents 0) {
if (epollEvent.data.fd == monitor) {
int fd = accept(monitor, NULL, 0);
+if (fd 0) {
+/* First reflex may be simply to declare accept failure
+ to be a fatal error. However, accept may fail when
+ a client quits between the above epoll_wait and here.
+ That case is not fatal, but rather to be expected,
+ if not common, so ignore it. */
+if (ignorable_epoll_accept_errno(errno))
+continue;
+virReportSystemError(errno, %s,
+ _(accept(monitor,...) failed));
+goto cleanup;
+}
if (client != -1) { /* Already connected, so kick new one out
*/
close(fd);
continue;
--
1.7.1.262.g5ef3d
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] maint: avoid unwanted newline at end of diagnostic
Jiri Denemark wrote: ... Subject: [PATCH 1/2] maint: remove unwanted newline at end of diagnostic * src/xen/xend_internal.c (xenDaemonDomainDefineXML): Remove \n. * src/network/bridge_driver.c (networkAddMasqueradingIptablesRules): Likewise. From 532672f3a0c53213609dab49df2e7efcaf0eb594 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Thu, 20 May 2010 11:13:41 +0200 Subject: [PATCH 2/2] maint: prohibit newline at end of diagnostic * cfg.mk (sc_prohibit_newline_at_end_of_diagnostic): New rule. Idea proposed by Jiri Denemark. ACK to both patches. While the syntax check implementation is not perfect, it's good enough for now and can be enhanced if there is a desperate need for that. Thanks. Pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirtd: don't ignore virInitialize failure
Chris Lalancette wrote:
Subject: [PATCH] libvirtd: don't ignore virInitialize failure
* daemon/libvirtd.c (main): Diagnose virInitialize failure
and exit nonzero.
---
daemon/libvirtd.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index afa10fb..aac2d08 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -3028,7 +3028,10 @@ int main(int argc, char **argv) {
{0, 0, 0, 0}
};
-virInitialize();
+if (virInitialize() 0) {
+fprintf (stderr, _(libvirtd: initialization failed\n));
+exit (EXIT_FAILURE);
+}
ACK
Thanks. Pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu driver: fix version parsing fix
Daniel P. Berrange wrote: ... That patch looks fine, and is nicely minimal. However, it does relax the test significantly. Rather than requiring that QEMU_VERSION_STR be a prefix, it would allow it to appear anywhere within the -help output. ... ACK, prefer the stricter check. One day QEMU might even provide this in a reliably parsable format like JSON... FYI, here's the complete patch I'll push in an hour or so. Note that I reordered, so that we check the new string before the old one. From 3b7179866bd9e495332335df74b327cc156a298c Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Thu, 20 May 2010 15:43:47 +0200 Subject: [PATCH] qemu_conf.c: also recognize new first line of qemu -help output * src/qemu/qemu_conf.c (QEMU_VERSION_STR_1, QEMU_VERSION_STR_2): Define these instead of... (QEMU_VERSION_STR): ... this. Remove definition. (qemudParseHelpStr): Check first for the new, shorter prefix, QEMU emulator version, and then for the old one, QEMU PC emulator version when trying to parse the version number. Based on a patch by Chris Wright. --- src/qemu/qemu_conf.c | 15 ++- 1 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5c14eb8..e1be340 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1246,7 +1246,9 @@ static unsigned long long qemudComputeCmdFlags(const char *help, /* We parse the output of 'qemu -help' to get the QEMU * version number. The first bit is easy, just parse - * 'QEMU PC emulator version x.y.z'. + * 'QEMU PC emulator version x.y.z' + * or + * 'QEMU emulator version x.y.z'. * * With qemu-kvm, however, that is followed by a string * in parenthesis as follows: @@ -1259,7 +1261,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STRQEMU PC emulator version +#define QEMU_VERSION_STR_1 QEMU emulator version +#define QEMU_VERSION_STR_2 QEMU PC emulator version #define QEMU_KVM_VER_PREFIX (qemu-kvm- #define KVM_VER_PREFIX (kvm- @@ -1277,11 +1280,13 @@ int qemudParseHelpStr(const char *qemu, *flags = *version = *is_kvm = *kvm_version = 0; -if (!STRPREFIX(p, QEMU_VERSION_STR)) +if (STRPREFIX(p, QEMU_VERSION_STR_1)) +p += strlen(QEMU_VERSION_STR_1); +else if (STRPREFIX(p, QEMU_VERSION_STR_2)) +p += strlen(QEMU_VERSION_STR_2); +else goto fail; -p += strlen(QEMU_VERSION_STR); - SKIP_BLANKS(p); major = virParseNumber(p); -- 1.7.1.262.g5ef3d -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] maint: avoid unwanted newline at end of diagnostic
Chris Lalancette wrote: ... - _(Failed to create inactive domain %s\n), def-name); + _(Failed to create inactive domain %s), def-name); goto error; } ACK to this part, certainly. I'm not sure a new syntax-check rule (which may have false positives) is worth it; the fact that there are so few occurrences of the problem in the codebase seems to say that it's not a huge problem, and I don't want to make sytnax-check fail for people for bogus reasons. Hi Chris, Thanks for the review. I already pushed it, based on a prior ack. If/when problems arise, we'll deal with it by improving the check, allowing exemption(s) via its .x-sc... file, or simply by removing the test altogether. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] VIR_ERROR and VIR_DEBUG normalization
Eric Blake wrote: On 05/20/2010 01:11 AM, Jim Meyering wrote: Most of the following changes have been induced automatically. The few outliers done manually are marked as such. [PATCH 01/10] maint: use VIR_ERROR0 rather than VIR_ERROR with a bare %s [PATCH 02/10] maint: mark translatable string args of VIR_ERROR0 [PATCH 03/10] maint: mark translatable string args of VIR_ERROR [PATCH 04/10] maint: VIR_ERROR/VIR_ERROR0: mark up the remaining ones manually [PATCH 05/10] maint: more of same, but manual: convert VIR_ERROR(%s to VIR_ERROR0( [PATCH 06/10] maint: change in err ? err-message : to _(unknown error), ... [PATCH 07/10] maint: enforce policy wrt VIR_ERROR and VIR_ERROR0 [PATCH 08/10] maint: don't mark VIR_DEBUG or VIR_DEBUG0 diagnostics for translation [PATCH 09/10] maint: enforce policy wrt VIR_DEBUG and VIR_DEBUG0 [PATCH 10/10] maint: update po/POTFILES.in I glanced through everything in the series, and didn't see any issues other than the possible fact that a 'make syntax-check' could fail when bisecting lands in the middle of the series due to a mismatch in po/POTFILES.in vs. translated strings. But I don't see any better way to arrange the series, so: Good catch. I debated whether to take the time to find the change set(s) that induced the requirement for those POTFILES.in additions, but didn't think it'd be worthwhile. ACK to all 10. Thanks for the review. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirtd: mark strings for translation, including --help output
Eric Blake wrote:
On 05/20/2010 02:02 AM, Jim Meyering wrote:
+++ b/daemon/libvirtd.c
@@ -484,8 +484,8 @@ static int daemonForkIntoBackground(void) {
if (ret == 1 status != 0) {
fprintf(stderr,
-error: %s. Check /var/log/messages or run without
---daemon for more info.\n,
+_(error: %s. Check /var/log/messages or run
without
+ --daemon for more info.\n),
Should we also do a followup that passes argv[0] to this method, so that
the error message can start with the program name?
Might as well.
patch below.
virDaemonErrTypeToString(status));
}
_exit(ret == 1 status == 0 ? 0 : 1);
@@ -2963,7 +2963,7 @@ static void
usage (const char *argv0)
{
fprintf (stderr,
- \n\
+ _(\n\
Usage:\n\
%s [options]\n\
As is the case in usage()?
+ (REMOTE_PID_FILE[0] != '\0'
+? REMOTE_PID_FILE
+: (disabled in ./configure)));
Missed a string. This should be _((disabled in ./configure)).
Thanks. Fixed in the v2 below.
default:
-fprintf (stderr, libvirtd: internal error: unknown flag: %c\n,
+fprintf (stderr, _(libvirtd: internal error: unknown flag:
%c\n),
And here, should we be using %s/argv[0] instead of hard-coding the name
libvirtd?
Yes.
From c5df9b5d7e29234b43d07f998971e4f61e24d0f1 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 10:01:32 +0200
Subject: [PATCH v2 1/2] libvirtd: mark strings for translation, including
--help output
* daemon/libvirtd.c (daemonForkIntoBackground, main): Mark strings
for translation.
(usage): Rework --help so that it is translatable, replacing
each embedded, configuration-dependent, macro with an `%s'.
libvirtd: don't ignore virInitialize failure
* daemon/libvirtd.c (main): Diagnose virInitialize failure
and exit nonzero.
---
daemon/libvirtd.c | 36 +---
1 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index aac2d08..be28165 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -484,8 +484,8 @@ static int daemonForkIntoBackground(void) {
if (ret == 1 status != 0) {
fprintf(stderr,
-error: %s. Check /var/log/messages or run without
---daemon for more info.\n,
+_(error: %s. Check /var/log/messages or run without
+ --daemon for more info.\n),
virDaemonErrTypeToString(status));
}
_exit(ret == 1 status == 0 ? 0 : 1);
@@ -2963,7 +2963,7 @@ static void
usage (const char *argv0)
{
fprintf (stderr,
- \n\
+ _(\n\
Usage:\n\
%s [options]\n\
\n\
@@ -2981,27 +2981,33 @@ libvirt management daemon:\n\
Default paths:\n\
\n\
Configuration file (unless overridden by -f):\n\
- SYSCONF_DIR /libvirt/libvirtd.conf\n\
+ %s/libvirt/libvirtd.conf\n\
\n\
Sockets (as root):\n\
- LOCAL_STATE_DIR /run/libvirt/libvirt-sock\n\
- LOCAL_STATE_DIR /run/libvirt/libvirt-sock-ro\n\
+ %s/run/libvirt/libvirt-sock\n\
+ %s/run/libvirt/libvirt-sock-ro\n\
\n\
Sockets (as non-root):\n\
$HOME/.libvirt/libvirt-sock (in UNIX abstract namespace)\n\
\n\
TLS:\n\
- CA certificate: LIBVIRT_CACERT \n\
- Server certificate: LIBVIRT_SERVERCERT \n\
- Server private key: LIBVIRT_SERVERKEY \n\
+ CA certificate: %s\n\
+ Server certificate: %s\n\
+ Server private key: %s\n\
\n\
PID file (unless overridden by --pid-file):\n\
%s\n\
-\n,
- argv0,
- REMOTE_PID_FILE[0] != '\0'
- ? REMOTE_PID_FILE
- : (disabled in ./configure));
+\n),
+ argv0,
+ SYSCONF_DIR,
+ LOCAL_STATE_DIR,
+ LOCAL_STATE_DIR,
+ LIBVIRT_CACERT,
+ LIBVIRT_SERVERCERT,
+ LIBVIRT_SERVERKEY,
+ (REMOTE_PID_FILE[0] != '\0'
+? REMOTE_PID_FILE
+: _((disabled in ./configure;
}
enum {
@@ -3083,7 +3089,7 @@ int main(int argc, char **argv) {
return 2;
default:
-fprintf (stderr, libvirtd: internal error: unknown flag: %c\n,
+fprintf (stderr, _(libvirtd: internal error: unknown flag: %c\n),
c);
exit (EXIT_FAILURE);
}
--
1.7.1.262.g5ef3d
From 2405307cc444d0ecdd6aa2326f29aee4f2375b49 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 21:40:54 +0200
Subject: [PATCH 2/2] libvirtd: use argv[0] in place of hard-coded libvirtd in
diagnostics
* daemon/libvirtd.c (main): Use argv[0
Re: [libvirt] [PATCH] libvirtd: mark strings for translation, including --help output
Eric Blake wrote:
On 05/20/2010 01:42 PM, Jim Meyering wrote:
Should we also do a followup that passes argv[0] to this method, so that
the error message can start with the program name?
Might as well.
patch below.
Didn't see any signature change to daemonForkIntoBackground...
Missed a string. This should be _((disabled in ./configure)).
Thanks. Fixed in the v2 below.
Confirmed.
default:
-fprintf (stderr, libvirtd: internal error: unknown flag:
%c\n,
+fprintf (stderr, _(libvirtd: internal error: unknown flag:
%c\n),
And here, should we be using %s/argv[0] instead of hard-coding the name
libvirtd?
Yes.
ACK to 1/2, but let's respin 2/2 to tweak daemonForkIntoBackground to
take another parameter...
Um...
Well there are two other diagnostics with no progname: prefix.
They're in qemudWritePidFile:
VIR_ERROR(_(Failed to write to pid file '%s' : %s)
VIR_ERROR(_(Failed to close pid file '%s' : %s)
so rather than adding parameters to two functions, I'll add a global,
argv0, and *remove* a few parameters:
From 3dde80f86d7a6afe59d5ae9c0dff70a592cf0c84 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Thu, 20 May 2010 21:40:54 +0200
Subject: [PATCH] libvirtd: start each diagnostic with argv0:
Some diagnostics had a hard-coded libvirtd: prefix, some used
error: and some used argv[0]: . Always use argv[0]: .
* daemon/libvirtd.c (argv0): New global.
(main): Set it.
(version, usage): Remove argv0 parameter. Use global; update callers.
(daemonForkIntoBackground): Use argv0:, not error:.
(qemudWritePidFile): Start each diagnostic with argv0:.
Suggested by Eric Blake.
---
daemon/libvirtd.c | 28 +++-
1 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index be28165..195c50a 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -190,6 +190,7 @@ static int max_client_requests = 5;
static sig_atomic_t sig_errors = 0;
static int sig_lasterrno = 0;
+static const char *argv0;
enum {
VIR_DAEMON_ERR_NONE = 0,
@@ -484,8 +485,8 @@ static int daemonForkIntoBackground(void) {
if (ret == 1 status != 0) {
fprintf(stderr,
-_(error: %s. Check /var/log/messages or run without
- --daemon for more info.\n),
+_(%s: error: %s. Check /var/log/messages or run
without
+ --daemon for more info.\n), argv0,
virDaemonErrTypeToString(status));
}
_exit(ret == 1 status == 0 ? 0 : 1);
@@ -515,15 +516,15 @@ static int qemudWritePidFile(const char *pidFile) {
}
if (fprintf(fh, %lu\n, (unsigned long)getpid()) 0) {
-VIR_ERROR(_(Failed to write to pid file '%s' : %s),
- pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+VIR_ERROR(_(%s: Failed to write to pid file '%s' : %s),
+ argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
fclose(fh);
return -1;
}
if (fclose(fh) == EOF) {
-VIR_ERROR(_(Failed to close pid file '%s' : %s),
- pidFile, virStrerror(errno, ebuf, sizeof ebuf));
+VIR_ERROR(_(%s: Failed to close pid file '%s' : %s),
+ argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf));
return -1;
}
@@ -2868,7 +2869,7 @@ remoteReadConfigFile (struct qemud_server *server, const
char *filename)
/* Display version information. */
static void
-version (const char *argv0)
+version (void)
{
printf (%s (%s) %s\n, argv0, PACKAGE_NAME, PACKAGE_VERSION);
}
@@ -2960,7 +2961,7 @@ error:
/* Print command-line usage. */
static void
-usage (const char *argv0)
+usage (void)
{
fprintf (stderr,
_(\n\
@@ -3021,6 +3022,7 @@ int main(int argc, char **argv) {
const char *remote_config_file = NULL;
int statuswrite = -1;
int ret = 1;
+argv0 = argv[0];
struct option opts[] = {
{ verbose, no_argument, verbose, 1},
@@ -3035,7 +3037,7 @@ int main(int argc, char **argv) {
};
if (virInitialize() 0) {
-fprintf (stderr, _(libvirtd: initialization failed\n));
+fprintf (stderr, _(%s: initialization failed\n), argv0);
exit (EXIT_FAILURE);
}
@@ -3081,16 +3083,16 @@ int main(int argc, char **argv) {
break;
case OPT_VERSION:
-version (argv[0]);
+version ();
return 0;
case '?':
-usage (argv[0]);
+usage ();
return 2;
default:
-fprintf (stderr, _(libvirtd: internal error: unknown flag: %c\n),
- c);
+fprintf (stderr, _(%s: internal error: unknown flag: %c\n),
+ argv0, c);
exit (EXIT_FAILURE);
}
}
--
1.7.1.262.g5ef3d
--
libvir-list mailing list
libvir-list
Re: [libvirt] [PATCH] correct typo: s/VIR_MIGRATE_TUNNELLED/VIR_MIGRATE_TUNNELED/, ...
Chris Lalancette wrote:
On 05/18/2010 01:15 PM, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, May 18, 2010 at 06:30:34PM +0200, Jim Meyering wrote:
I noticed a typo in a public interface.
IMHO it's well worth fixing, so propose this:
(imagine someone searching for all occurrences of tunneled
with the proper spelling. They would miss this symbol.)
I don't think we should make this kind of change. It has
no functional gain for app developers, but adds the downside
that anyone using this new symbol has needlessly made their
code incompatible with libvirt 0.7.5, 0.8.0, 0.8.1, etc, etc
There are plenty of new features that people will be using
that will render their code incompatible with previous releases.
However, if you insist, let's at least mark it as a known error
so that searches for the properly spelled symbol will turn up
the misspelled one:
From 8ae4cc7d25efaab531f5045940c13ec3bb36497f Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 19:13:20 +0200
Subject: [PATCH] note a typo: VIR_MIGRATE_TUNNELLED should be
VIR_MIGRATE_TUNNELED,
so that searches for properly spelled TUNNELED will turn up
the misspelled symbol name.
* include/libvirt/libvirt.h.in: Add a comment.
...
+/* note the spelling error that we're stuck with:
+ VIR_MIGRATE_TUNNELLED should be VIR_MIGRATE_TUNNELED */
TUNNELLED is not wrong:
http://www.thefreedictionary.com/tunnelled
It's an acceptable alternate spelling.
tunnelled is not acceptable to spelling checkers (spell, aspell, hunspell).
It is by far the less-common of those two spellings, so libvirt should
avoid using it. Adding the shim of a comment is the least we can do.
I've adjusted the comment and the log not to call it an error
and pushed this:
From 8a8c257bf0601eba64f285c33ce70055e72b4d44 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 19:13:20 +0200
Subject: [PATCH] note a typo: VIR_MIGRATE_TUNNELLED should be
VIR_MIGRATE_TUNNELED,
so that searches for properly spelled TUNNELED turn up
the less common spelling.
* include/libvirt/libvirt.h.in: Add a comment.
---
include/libvirt/libvirt.h.in |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 838028d..1ff7df0 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -4,7 +4,7 @@
* Description: Provides the interfaces of the libvirt library to handle
* virtualized domains
*
- * Copy: Copyright (C) 2005,2006 Red Hat, Inc.
+ * Copy: Copyright (C) 2005,2006,2010 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -407,6 +407,8 @@ typedef enum {
typedef enum {
VIR_MIGRATE_LIVE = (1 0), /* live migration */
VIR_MIGRATE_PEER2PEER = (1 1), /* direct source - dest host
control channel */
+/* Note the less-common spelling that we're stuck with:
+ VIR_MIGRATE_TUNNELLED should be VIR_MIGRATE_TUNNELED */
VIR_MIGRATE_TUNNELLED = (1 2), /* tunnel migration data over
libvirtd connection */
VIR_MIGRATE_PERSIST_DEST = (1 3), /* persist the VM on the
destination */
VIR_MIGRATE_UNDEFINE_SOURCE = (1 4), /* undefine the VM on the source
*/
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Problems with pdwtags on Ubuntu 10.04
Matthias Bolte wrote:
The help avoid accidental remote_protocol.x changes commit 180d4b2b
added a make check rule that tried using pdwtags from the dwarves
package to protect against accidental remote_protocol.x changes.
I installed dwarves package on Ubuntu 10.04 and make check fails for me now.
The temporary file remote_protocol-structs-t is empty for me. It seems
that pdwtags doesn't output the expected format for the embedded perl
script. pdwtags output doesn't contain /* DD */ comments between the
structs. A snippet from the pdwtags output looks like this:
Thanks for the report.
At first I thought it might be worthwhile to
adjust the splitting code to accommodate 1.3 with --verbose:
-e 'foreach my $$p (split m!\n\n/\* (?:\d+|\S+ \S+) \*/\n!) {'\
That works with pdwtags --verbose when it's 1.3,
but with 1.8.x, as Eric noted, we get yet another variant:
/* 93 */
/* 0 (null):0 */
which is not matched by the above.
I could match only the lines with the hex-digit ...:\d+ comments,
but prefer to use a tighter regexp (albeit more involved) so that I can
continue to require a blank line (the \n\n) just before the separator.
Matthias,
Would you please verify that this solves the problem
when using your older pdwtags program?
From a8d8ff6ba4791972483093215291eef5fa87cf5d Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 19 May 2010 15:36:27 +0200
Subject: [PATCH] tests: the remote_protocol check also accommodates older
pdwtags
This test was failing on systems using pdwtags from dwarves-1.3.
Reported by Matthias Bolte.
Two-pronged fix:
- use --verbose to work also with dwarves-1.3; adapt regular
expressions to handle now-varying separators
- require a minimum number of post-split clauses, in order to
skip upon any future format change.
Currently there are 318; if there are 300 or fewer,
give a warning similar to when pdwtags is missing.
* src/Makefile.am (remote_protocol-structs): Use pdwtags' --verbose
option to make 1.3 emit member sizes and offsets.
Consistently output WARNING messages to stderr.
---
src/Makefile.am | 40 +++-
1 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index 889de8e..7ddf6aa 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -167,22 +167,44 @@ EXTRA_DIST += remote/remote_protocol.x
remote/rpcgen_fix.pl
# * remove comments and preceding TAB throughout
# * remove empty lines throughout
# * remove white space at end of buffer
+
+# With pdwtags 1.8, --verbose output includes separators like these:
+# /* 93 */
+# /* 0 (null):0 */
+# whereas with pdwtags 1.3, they look like this:
+# /* 2d2 /usr/include/libio.h:180 */
+# The concatenation of the following regexps matches both cases.
+r1 = (?:/\* \d+ \*/\n)?
+r2 = /\* [[:xdigit:]]+ \S+:\d+ \*/
+
.PHONY: remote_protocol-structs
remote_protocol-structs:
$(AM_V_GEN)if (pdwtags --help) /dev/null 21; then \
- pdwtags libvirt_driver_remote_la-remote_protocol.$(OBJEXT)\
+ pdwtags --verbose libvirt_driver_remote_la-remote_protocol.$(OBJEXT)
\
| perl -0777 -n \
- -e 'foreach my $$p (split m!\n\n/\* \d+ \*/\n!)'\
- -e ' { if ($$p =~ /^struct remote_/) {'\
- -e ' $$p =~ s!\t*/\*.*?\*/!!sg;' \
- -e ' $$p =~ s!\s+\n!\n!sg;'\
- -e ' $$p =~ s!\s+$$!!;'\
- -e ' print $$p\n } }'\
+ -e 'foreach my $$p (split m!\n\n$(r1)$(r2)\n!) {' \
+ -e ' if ($$p =~ /^struct remote_/) {' \
+ -e '$$p =~ s!\t*/\*.*?\*/!!sg;' \
+ -e '$$p =~ s!\s+\n!\n!sg;' \
+ -e '$$p =~ s!\s+$$!!;' \
+ -e 'print $$p\n;' \
+ -e '$$n++;' \
+ -e ' }'\
+ -e '}' \
+ -e 'END {' \
+ -e ' if ($$n 300) {' \
+ -e 'warn WARNING: your pdwtags program is too old\n;' \
+ -e 'warn WARNING: skipping the $@ test\n;'\
+ -e 'warn WARNING: install dwarves-1.8 or newer\n;' \
+ -e 'exit 8;'\
+ -e ' }'\
+ -e '}' \
$...@-t
Re: [libvirt] VIR_WARN and VIR_WARN0: many diags were not marked for translation
Jiri Denemark wrote:
Nonetheless, many of my manual changes highlighted
existing (bogus) attempts to translate VIR_WARN0? diagnostics.
Those should be addressed regardless.
Considering the large number of existing VIR_WARN uses with strings
marked for translation, do you want to reconsider?
$ git grep 'VIR_WARN.*_('|wc -l
60
or would you prefer to remove the _(...) marks on those 60 uses?
Yes, pretty much all of them can be removed. I notice there are also needless
translations in a handful of files with VIR_DEBUG, and for VIR_ERROR too.
IIRC, there was an agreement on the list that VIR_ERROR should be left
translated since those messages can make their way to end users.
Thanks for the clarification.
I've done the VIR_WARN* normalization (will post shortly)
and will eventually do the following, too:
ensure that all VIR_ERROR diagnostics are marked
ensure that *no* VIR_DEBUG diagnostic is marked
That is consistent with existing trends:
- very few VIR_DEBUG diagnostics are marked, and
- barely 1/3 of the VIR_ERROR diagnostics are unmarked
$ git grep 'VIR_ERROR\s*('|wc -l
67
$ git grep 'VIR_ERROR\s*(_('|wc -l
110
$ git grep 'VIR_DEBUG\s*('|wc -l
233
$ git grep 'VIR_DEBUG\s*(_('|wc -l
26
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] maint: use VIR_WARN0(...) rather than VIR_WARN(%s, ...)
From: Jim Meyering meyer...@redhat.com
Run this command:
git grep -l 'VIR_WARN(%s, '|xargs perl -pi -e \
's/VIR_WARN\(%s, /VIR_WARN0(/'
* src/phyp/phyp_driver.c (phypDomainGetInfo): Perform the above.
(phypDomainCreateAndStart, phypUUIDTable_ReadFile): Likewise.
---
src/phyp/phyp_driver.c | 12 ++--
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index cec99b1..b174fc9 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1344,15 +1344,15 @@ phypDomainGetInfo(virDomainPtr dom, virDomainInfoPtr
info)
if ((info-maxMem =
phypGetLparMem(dom-conn, managed_system, dom-id, 0)) == 0)
-VIR_WARN(%s, Unable to determine domain's max memory.);
+VIR_WARN0(Unable to determine domain's max memory.);
if ((info-memory =
phypGetLparMem(dom-conn, managed_system, dom-id, 1)) == 0)
-VIR_WARN(%s, Unable to determine domain's memory.);
+VIR_WARN0(Unable to determine domain's memory.);
if ((info-nrVirtCpu =
phypGetLparCPU(dom-conn, managed_system, dom-id)) == 0)
-VIR_WARN(%s, Unable to determine domain's CPU.);
+VIR_WARN0(Unable to determine domain's CPU.);
return 0;
}
@@ -1416,14 +1416,14 @@ phypDomainCreateAndStart(virConnectPtr conn,
/* checking if this name already exists on this system */
if (phypGetLparID(session, managed_system, def-name, conn) == -1) {
-VIR_WARN(%s, LPAR name already exists.);
+VIR_WARN0(LPAR name already exists.);
goto err;
}
/* checking if ID or UUID already exists on this system */
for (i = 0; i uuid_table-nlpars; i++) {
if (lpars[i]-id == def-id || lpars[i]-uuid == def-uuid) {
-VIR_WARN(%s, LPAR ID or UUID already exists.);
+VIR_WARN0(LPAR ID or UUID already exists.);
goto err;
}
}
@@ -1782,7 +1782,7 @@ phypUUIDTable_ReadFile(virConnectPtr conn)
int id;
if ((fd = open(local_file, O_RDONLY)) == -1) {
-VIR_WARN(%s, Unable to write information to local file.);
+VIR_WARN0(Unable to write information to local file.);
goto err;
}
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] maint: don't mark VIR_WARN or VIR_WARN0 diagnostics for translation
From: Jim Meyering meyer...@redhat.com
Approximately 60 messages were marked. Since these diagnostics are
intended solely for developers and maintainers, encouraging translation
is deemed to be counterproductive:
http://thread.gmane.org/gmane.comp.emulators.libvirt/25050/focus=25052
Run this command:
git grep -l VIR_WARN|xargs perl -pi -e \
's/(VIR_WARN0?)\s*\(_\((.*?)\)/$1($2/'
---
daemon/libvirtd.c|6 +++---
src/lxc/lxc_container.c |2 +-
src/lxc/lxc_controller.c |2 +-
src/network/bridge_driver.c |8
src/qemu/qemu_conf.c | 12 ++--
src/qemu/qemu_driver.c | 32
src/qemu/qemu_monitor_text.c | 22 +++---
src/uml/uml_driver.c | 12 ++--
src/util/logging.c |8
src/util/pci.c |8
src/xen/proxy_internal.c |6 +++---
11 files changed, 59 insertions(+), 59 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4533f40..cc05953 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -384,14 +384,14 @@ qemudDispatchSignalEvent(int watch ATTRIBUTE_UNUSED,
virHookCall(VIR_HOOK_DRIVER_DAEMON, -,
VIR_HOOK_DAEMON_OP_RELOAD, SIGHUP, SIGHUP, NULL);
if (virStateReload() 0)
-VIR_WARN0(_(Error while reloading drivers));
+VIR_WARN0(Error while reloading drivers);
break;
case SIGINT:
case SIGQUIT:
case SIGTERM:
-VIR_WARN(_(Shutting down on signal %d), siginfo.si_signo);
+VIR_WARN(Shutting down on signal %d, siginfo.si_signo);
server-quitEventThread = 1;
break;
@@ -2761,7 +2761,7 @@ remoteReadConfigFile (struct qemud_server *server, const
char *filename)
GET_CONF_STR (conf, filename, unix_sock_group);
if (unix_sock_group) {
if (!server-privileged) {
-VIR_WARN0(_(Cannot set group when not running as root));
+VIR_WARN0(Cannot set group when not running as root);
} else {
int ret;
struct group grpdata, *grp;
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 706c796..018f4d5 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -716,7 +716,7 @@ static int lxcContainerDropCapabilities(void)
* be unmasked - they can never escape the bounding set. */
#else
-VIR_WARN0(_(libcap-ng support not compiled in, unable to clear
capabilities));
+VIR_WARN0(libcap-ng support not compiled in, unable to clear
capabilities);
#endif
return 0;
}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index c120b8e..1732780 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -259,7 +259,7 @@ static int lxcControllerClearCapabilities(void)
return -1;
}
#else
-VIR_WARN0(_(libcap-ng support not compiled in, unable to clear
capabilities));
+VIR_WARN0(libcap-ng support not compiled in, unable to clear
capabilities);
#endif
return 0;
}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 8432bbc..3b9b4f4 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -995,14 +995,14 @@ static int networkStartNetworkDaemon(struct
network_driver *driver,
err_delbr1:
if ((err = brSetInterfaceUp(driver-brctl, network-def-bridge, 0))) {
char ebuf[1024];
-VIR_WARN(_(Failed to bring down bridge '%s' : %s),
+VIR_WARN(Failed to bring down bridge '%s' : %s,
network-def-bridge, virStrerror(err, ebuf, sizeof ebuf));
}
err_delbr:
if ((err = brDeleteBridge(driver-brctl, network-def-bridge))) {
char ebuf[1024];
-VIR_WARN(_(Failed to delete bridge '%s' : %s),
+VIR_WARN(Failed to delete bridge '%s' : %s,
network-def-bridge, virStrerror(err, ebuf, sizeof ebuf));
}
@@ -1035,12 +1035,12 @@ static int networkShutdownNetworkDaemon(struct
network_driver *driver,
char ebuf[1024];
if ((err = brSetInterfaceUp(driver-brctl, network-def-bridge, 0))) {
-VIR_WARN(_(Failed to bring down bridge '%s' : %s),
+VIR_WARN(Failed to bring down bridge '%s' : %s,
network-def-bridge, virStrerror(err, ebuf, sizeof ebuf));
}
if ((err = brDeleteBridge(driver-brctl, network-def-bridge))) {
-VIR_WARN(_(Failed to delete bridge '%s' : %s),
+VIR_WARN(Failed to delete bridge '%s' : %s,
network-def-bridge, virStrerror(err, ebuf, sizeof ebuf));
}
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 5fa8c0a..3e334dc 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -523,7 +523,7 @@ rewait:
* as there's really no need to throw an error if we did
* actually read a valid version number above */
if (WEXITSTATUS(status) != 0) {
-VIR_WARN(_(Unexpected exit status '%d', qemu
[libvirt] [PATCH 5/5] maint: more VIR_WARN corrections: now manually
From: Jim Meyering meyer...@redhat.com
* po/POTFILES.in: Remove src/util/logging.c and src/util/uuid.c.
* src/phyp/phyp_driver.c (phypUUIDTable_ReadFile): Correct more
VIR_WARN uses, now manually.
(phypUUIDTable_Init, phypUUIDTable_Pull): Likewise.
---
po/POTFILES.in |2 --
src/phyp/phyp_driver.c | 12
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 88218bd..e08b8c8 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -76,14 +76,12 @@ src/util/hooks.c
src/util/hostusb.c
src/util/interface.c
src/util/json.c
-src/util/logging.c
src/util/macvtap.c
src/util/pci.c
src/util/processinfo.c
src/util/stats_linux.c
src/util/storage_file.c
src/util/util.c
-src/util/uuid.c
src/util/virterror.c
src/util/xml.c
src/vbox/vbox_driver.c
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index b174fc9..8f4f310 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1798,15 +1798,13 @@ phypUUIDTable_ReadFile(virConnectPtr conn)
}
uuid_table-lpars[i]-id = id;
} else {
-VIR_WARN(%s,
- Unable to read from information to local file.);
+VIR_WARN0(Unable to read from information to local file.);
goto err;
}
rc = read(fd, uuid_table-lpars[i]-uuid, VIR_UUID_BUFLEN);
if (rc != VIR_UUID_BUFLEN) {
-VIR_WARN(%s,
- Unable to read information to local file.);
+VIR_WARN0(Unable to read information to local file.);
goto err;
}
}
@@ -1909,8 +1907,7 @@ phypUUIDTable_Init(virConnectPtr conn)
uuid_table-lpars[i]-id = ids[i];
if (virUUIDGenerate(uuid_table-lpars[i]-uuid) 0)
-VIR_WARN(%s %d, Unable to generate UUID for domain,
- ids[i]);
+VIR_WARN(Unable to generate UUID for domain %d, ids[i]);
}
} else {
virReportOOMError();
@@ -2083,8 +2080,7 @@ phypUUIDTable_Pull(virConnectPtr conn)
rc = libssh2_channel_read(channel, buffer, amount);
if (rc 0) {
if (safewrite(fd, buffer, rc) != rc)
-VIR_WARN(%s,
- Unable to write information to local file.);
+VIR_WARN0(Unable to write information to local file.);
got += rc;
total += rc;
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] maint: enforce no-markup policy wrt VIR_WARN-like macros
From: Jim Meyering meyer...@redhat.com
* cfg.mk (sc_prohibit_gettext_markup): New rule, to enforce
this policy. Contrary to most diagnostic-emitting functions,
where we require _(...) markup, here, we require that _() *not*
be used for certain functions (or function-like macros).
---
cfg.mk | 19 +--
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 96d6953..04719d4 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -228,7 +228,7 @@ sc_avoid_write:
@prohibit='\write *\(' \
in_vc_files='\.c$$' \
halt='consider using safewrite instead of write'\
-$(_sc_search_regexp)
+ $(_sc_search_regexp)
# Use STREQ rather than comparing strcmp == 0, or != 0.
# Similarly, use STREQLEN or STRPREFIX rather than strncmp.
@@ -285,7 +285,7 @@ sc_prohibit_nonreentrant:
sc_prohibit_ctype_h:
@prohibit='^# *include *ctype\.h'\
halt=don't use ctype.h; instead, use c-ctype.h\
-$(_sc_search_regexp)
+ $(_sc_search_regexp)
# Ensure that no C source file or rng schema uses TABs for
# indentation. Also match *.h.in files, to get libvirt.h.in. Exclude
@@ -294,7 +294,7 @@ sc_TAB_in_indentation:
@prohibit='^ * ' \
in_vc_files='(\.(rng|[ch](\.in)?)|(daemon|tools)/.*\.in)$$' \
halt='use spaces, not TAB, for indentation in C, sh, and RNG schemas' \
-$(_sc_search_regexp)
+ $(_sc_search_regexp)
ctype_re = isalnum|isalpha|isascii|isblank|iscntrl|isdigit|isgraph|islower\
|isprint|ispunct|isspace|isupper|isxdigit|tolower|toupper
@@ -302,7 +302,7 @@ ctype_re =
isalnum|isalpha|isascii|isblank|iscntrl|isdigit|isgraph|islower\
sc_avoid_ctype_macros:
@prohibit='\b($(ctype_re)) *\(' \
halt=don't use ctype macros (use c-ctype.h) \
-$(_sc_search_regexp)
+ $(_sc_search_regexp)
sc_prohibit_virBufferAdd_with_string_literal:
@prohibit='\virBufferAdd *\([^,]+, *[^]' \
@@ -415,10 +415,17 @@ sc_copyright_format:
@require='Copyright .*Red 'Hat', Inc\.' \
containing='Copyright .*Red 'Hat\
halt='Red Hat copyright is missing Inc.'\
-$(_sc_search_regexp)
+ $(_sc_search_regexp)
@prohibit='Copyright [^(].*Red 'Hat \
halt='consistently use (C) in Red Hat copyright'\
-$(_sc_search_regexp)
+ $(_sc_search_regexp)
+
+# Some functions/macros produce messages intended solely for developers
+# and maintainers. Do not mark them for translation.
+sc_prohibit_gettext_markup:
+ @prohibit='\VIR_WARN0? *\(_\(' \
+ halt='do not mark these strings for translation'\
+ $(_sc_search_regexp)
# We don't use this feature of maint.mk.
prev_version_file = /dev/null
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] maint: remove _(...) from VIR_WARN arg manually
From: Jim Meyering meyer...@redhat.com
* src/util/uuid.c (virUUIDGenerate): Remove _(...) manually.
---
src/util/uuid.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 459273a..9c626ce 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2007, 2008, 2009 Red Hat, Inc.
+ * Copyright (C) 2007, 2008, 2009, 2010 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
@@ -101,8 +101,8 @@ virUUIDGenerate(unsigned char *uuid)
if ((err = virUUIDGenerateRandomBytes(uuid, VIR_UUID_BUFLEN))) {
char ebuf[1024];
-VIR_WARN(_(Falling back to pseudorandom UUID,
-failed to generate random bytes: %s),
+VIR_WARN(Falling back to pseudorandom UUID,
+ failed to generate random bytes: %s,
virStrerror(err, ebuf, sizeof ebuf));
err = virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN);
}
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] VIR_WARN and VIR_WARN0 normalization
Here are 5 patches, mostly mechanical, to straighten out the VIR_WARN and VIR_WARN0 situation. 1: mechanically remove all _(...) uses in VIR_WARN*. 2: remove the few remaining _(...) uses manually 3: add syntax-check rule to enforce the policy 4 clean up bogosity I noticed along the way, first automatically, 5 ... then manually b/cfg.mk | 19 +-- b/daemon/libvirtd.c|6 +++--- b/po/POTFILES.in |2 -- b/src/lxc/lxc_container.c |2 +- b/src/lxc/lxc_controller.c |2 +- b/src/network/bridge_driver.c |8 b/src/phyp/phyp_driver.c | 12 ++-- b/src/qemu/qemu_conf.c | 12 ++-- b/src/qemu/qemu_driver.c | 32 b/src/qemu/qemu_monitor_text.c | 22 +++--- b/src/uml/uml_driver.c | 12 ++-- b/src/util/logging.c |8 b/src/util/pci.c |8 b/src/util/uuid.c |6 +++--- b/src/xen/proxy_internal.c |6 +++--- src/phyp/phyp_driver.c | 12 16 files changed, 85 insertions(+), 84 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] maint: don't mark VIR_WARN or VIR_WARN0 diagnostics for translation
Daniel P. Berrange wrote: On Wed, May 19, 2010 at 04:47:16PM +0200, Jim Meyering wrote: From: Jim Meyering meyer...@redhat.com Approximately 60 messages were marked. Since these diagnostics are intended solely for developers and maintainers, encouraging translation is deemed to be counterproductive: http://thread.gmane.org/gmane.comp.emulators.libvirt/25050/focus=25052 Run this command: git grep -l VIR_WARN|xargs perl -pi -e \ 's/(VIR_WARN0?)\s*\(_\((.*?)\)/$1($2/' --- daemon/libvirtd.c|6 +++--- src/lxc/lxc_container.c |2 +- src/lxc/lxc_controller.c |2 +- src/network/bridge_driver.c |8 src/qemu/qemu_conf.c | 12 ++-- src/qemu/qemu_driver.c | 32 src/qemu/qemu_monitor_text.c | 22 +++--- src/uml/uml_driver.c | 12 ++-- src/util/logging.c |8 src/util/pci.c |8 src/xen/proxy_internal.c |6 +++--- 11 files changed, 59 insertions(+), 59 deletions(-) ACK Thanks for the quick reviews. And for trimming the quoted text. I've pushed all 5. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemudDomainMigrateFinish2: handle a case of virDomainSaveStatus failure
This is very similar to ones I fixed yesterday.
The difference is that I'm adding ATTRIBUTE_RETURN_CHECK, now.
Here, it's easy, since the function is private.
From cfce459e9aebae840356e62473f550f358834f30 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 19 May 2010 17:48:03 +0200
Subject: [PATCH] qemudDomainMigrateFinish2: handle a case of
virDomainSaveStatus failure
* src/qemu/qemu_driver.c (qemudDomainMigrateFinish2): Don't ignore
virDomainSaveStatus failure.
* src/conf/domain_conf.h (virDomainSaveStatus): Use
ATTRIBUTE_RETURN_CHECK, so this doesn't happen again.
---
src/conf/domain_conf.h |2 +-
src/qemu/qemu_driver.c |5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fadc8bd..a7206d3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1011,7 +1011,7 @@ int virDomainSaveConfig(const char *configDir,
virDomainDefPtr def);
int virDomainSaveStatus(virCapsPtr caps,
const char *statusDir,
-virDomainObjPtr obj);
+virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK;
typedef void (*virDomainLoadConfigNotify)(virDomainObjPtr dom,
int newDomain,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 65ca117..a519c02 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10816,7 +10816,10 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn,
VIR_DOMAIN_EVENT_SUSPENDED,
VIR_DOMAIN_EVENT_SUSPENDED_PAUSED);
}
-virDomainSaveStatus(driver-caps, driver-stateDir, vm);
+if (virDomainSaveStatus(driver-caps, driver-stateDir, vm) 0) {
+VIR_WARN(Failed to save status on vm %s, vm-def-name);
+goto endjob;
+}
} else {
qemudShutdownVMDaemon(driver, vm, 0);
event = virDomainEventNewFromObj(vm,
--
1.7.1.259.g3aef8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Problems with pdwtags on Ubuntu 10.04
Matthias Bolte wrote: ... Yes, this patch solves the problem. ... + -e ' warn WARNING: your pdwtags program is too old\n;' \ + -e ' warn WARNING: skipping the $@ test\n;' \ + -e ' warn WARNING: install dwarves-1.8 or newer\n;' \ Maybe the warning should suggest dwarves-1.3 as minimum version, because this patch makes it work with dwarves-1.3. Good point. That was from an earlier (abandoned) version that required a version of pdwtags that accepts --version. Adjusted and pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: distribute more documentation
Eric Blake wrote: * src/Makefile.am (EXTRA_DIST): Add THREADS.txt. --- I noticed this while working on back-porting a patch to RHEL-6 - the qemu tree in my git repository had more files than were in the unpacked 0.8.1 tarball. src/Makefile.am |3 ++- 1 file changed, 2 insertions(+), 1 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 889de8e..4c9fdf8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -588,7 +588,8 @@ augeas_DATA += qemu/libvirtd_qemu.aug augeastest_DATA += qemu/test_libvirtd_qemu.aug endif -EXTRA_DIST += qemu/qemu.conf qemu/libvirtd_qemu.aug qemu/test_libvirtd_qemu.aug +EXTRA_DIST += qemu/qemu.conf qemu/libvirtd_qemu.aug \ + qemu/test_libvirtd_qemu.aug qemu/THREADS.txt Obviously correct. Though if you add THREADS.txt, maybe you want to add daemon/THREADING.txt, too: $ git ls-files|grep THREAD daemon/THREADING.txt src/qemu/THREADS.txt $ grep THREA daemon/M*.am [Exit 1] $ ACK either way. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] initialize meta in virStorageFileGetMetadata, not in each caller
Here's proactive clean-up that is IMHO required.
Otherwise, it's just too easy not to realize that meta must
be cleared before each and every call to virStorageFileGetMetadata and
virStorageFileGetMetadataFromFD.
Besides, any change that adds just 2 lines and removes 11,
in addition to making the code harder to abuse must be an improvement.
From 837732f1e307208b52721ffba83102e0edc361a7 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 07:53:31 +0200
Subject: [PATCH] initialize meta in virStorageFileGetMetadata, not in each
caller
Do not require each caller of virStorageFileGetMetadata and
virStorageFileGetMetadataFromFD to first clear the storage of the
meta buffer. Instead, initialize that storage in
virStorageFileGetMetadataFromFD.
* src/util/storage_file.c (virStorageFileGetMetadataFromFD): Clear
meta here, not before each of the following callers.
* src/qemu/qemu_driver.c (qemuSetupDiskCgroup): Don't clear meta here.
(qemuTeardownDiskCgroup): Likewise.
* src/qemu/qemu_security_dac.c (qemuSecurityDACSetSecurityImageLabel):
Likewise.
* src/security/security_selinux.c (SELinuxSetSecurityImageLabel):
Likewise.
* src/security/virt-aa-helper.c (get_files): Likewise.
---
src/qemu/qemu_driver.c |5 -
src/qemu/qemu_security_dac.c|2 --
src/security/security_selinux.c |2 --
src/security/virt-aa-helper.c |2 --
src/util/storage_file.c |2 ++
5 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e025987..0e70b1b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2982,8 +2982,6 @@ static int qemuSetupDiskCgroup(virCgroupPtr cgroup,
}
}
-memset(meta, 0, sizeof(meta));
-
rc = virStorageFileGetMetadata(path, meta);
if (path != disk-src)
@@ -3030,8 +3028,6 @@ static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
}
}
-memset(meta, 0, sizeof(meta));
-
rc = virStorageFileGetMetadata(path, meta);
if (path != disk-src)
@@ -9316,7 +9312,6 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
}
/* Probe for magic formats */
-memset(meta, 0, sizeof(meta));
if (virStorageFileGetMetadataFromFD(path, fd, meta) 0)
goto cleanup;
diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c
index a816441..52150f7 100644
--- a/src/qemu/qemu_security_dac.c
+++ b/src/qemu/qemu_security_dac.c
@@ -115,8 +115,6 @@ qemuSecurityDACSetSecurityImageLabel(virDomainObjPtr vm
ATTRIBUTE_UNUSED,
virStorageFileMetadata meta;
int ret;
-memset(meta, 0, sizeof(meta));
-
ret = virStorageFileGetMetadata(path, meta);
if (path != disk-src)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 669ef42..d90e17c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -454,8 +454,6 @@ SELinuxSetSecurityImageLabel(virDomainObjPtr vm,
virStorageFileMetadata meta;
int ret;
-memset(meta, 0, sizeof(meta));
-
ret = virStorageFileGetMetadata(path, meta);
if (path != disk-src)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 88cdc9d..c66f107 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -830,8 +830,6 @@ get_files(vahControl * ctl)
do {
virStorageFileMetadata meta;
-memset(meta, 0, sizeof(meta));
-
ret = virStorageFileGetMetadata(path, meta);
if (path != ctl-def-disks[i]-src)
diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index a07bedc..b3ae905 100644
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -275,6 +275,8 @@ virStorageFileGetMetadataFromFD(const char *path,
unsigned char head[20*512]; /* vmdk4GetBackingStore needs this much. */
int len, i;
+memset(meta, 0, sizeof (*meta));
+
/* If all else fails, call it a raw file */
meta-format = VIR_STORAGE_FILE_RAW;
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemudDomainSetVcpus: avoid NULL-deref on failed uuid look-up
Eric Blake wrote: On 05/17/2010 11:33 AM, Jim Meyering wrote: Here's the fix, followed by the endjob/cleanup code: From d696f6067d6e802714adbf3e36bf53c9fcf3ec76 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Mon, 17 May 2010 19:28:44 +0200 Subject: [PATCH] qemudDomainSetVcpus: avoid NULL-deref on failed uuid look-up * src/qemu/qemu_driver.c (qemudDomainSetVcpus): Upon look-up failure, i.e., vm==NULL, goto cleanup, rather than to endjob, superficially since the latter would dereference vm, but more fundamentally because we certainly don't want to call qemuDomainObjEndJob before we've even attempted qemuDomainObjBeginJob. ACK. Thanks. Pushed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] ebiptablesWriteToTempFile: don't close a negative file descriptor
If mkstemp fails here, we end up closing a negative FD:
int fd = mkstemp(filename);
if (fd 0) {
virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
%s,
_(cannot create temporary file));
goto err_exit;
}
Here's the fix:
From b7c6593b3a8b59d49b492cd45fbf5f9c706bb78f Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 10:11:23 +0200
Subject: [PATCH] ebiptablesWriteToTempFile: don't close a negative file
descriptor
* src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesWriteToTempFile):
Skip the close if fd is negative.
---
src/nwfilter/nwfilter_ebiptables_driver.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
b/src/nwfilter/nwfilter_ebiptables_driver.c
index 63bcbd7..ae21906 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -2245,7 +2245,8 @@ ebiptablesWriteToTempFile(const char *string) {
err_exit:
VIR_FREE(header);
-close(fd);
+if (fd = 0)
+close(fd);
unlink(filename);
return NULL;
}
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virDomainNetDefParseXML: avoid leak upon multiple filterref
The offending code below appears in this loop:
virNWFilterHashTablePtr filterparams = NULL;
...
cur = node-children;
while (cur != NULL) {
...
}
so the first assignment works fine, but second and subsequent
ones leak the buffer returned by each preceding
virNWFilterParseParamAttributes call.
From 8659fb1ae879befe360e1ec7b8b62434c22698cd Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 10:19:30 +0200
Subject: [PATCH] virDomainNetDefParseXML: avoid leak upon multiple filterref
* src/conf/domain_conf.c (virDomainNetDefParseXML): Don't leak
memory when parsing two or more filterref elements.
---
src/conf/domain_conf.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e45f79..0c717f2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1905,6 +1905,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
model = virXMLPropString(cur, type);
} else if (xmlStrEqual (cur-name, BAD_CAST filterref)) {
filter = virXMLPropString(cur, filter);
+free(filterparams);
filterparams = virNWFilterParseParamAttributes(cur);
} else if ((flags VIR_DOMAIN_XML_INTERNAL_STATUS)
xmlStrEqual(cur-name, BAD_CAST state)) {
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] linuxNodeInfoCPUPopulate: avoid used-uninitialized via a test
See the comment:
From 1b200ba22d742e685de0b9853c8cd276df8e129f Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 11:58:32 +0200
Subject: [PATCH] linuxNodeInfoCPUPopulate: avoid used-uninitialized via a test
* tests/nodeinfotest.c (linuxTestCompareFiles): Don't use
nodeinfo-member uninitialized. linuxNodeInfoCPUPopulate requires
that some of its nodeinfo members (including threads) be initialized
upon input. The nodeinfotest.c program lacked the initialization,
while the only other use (nodeGetInfo) did perform it.
It's not trivial to move the initialization into the function,
since nodeGetInfo sets at least one member after clearing the
buffer but before calling linuxNodeInfoCPUPopulate.
---
tests/nodeinfotest.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index 9aeb459..cb92215 100644
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -40,6 +40,8 @@ static int linuxTestCompareFiles(const char *cpuinfofile,
const char *outputfile
cpuinfo = fopen(cpuinfofile, r);
if (!cpuinfo)
return -1;
+
+memset(nodeinfo, 0, sizeof(*nodeinfo));
if (linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo) 0) {
fclose(cpuinfo);
return -1;
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virNWFilterDefParseXML: avoid leak on error paths
In this function,
static virNWFilterDefPtr
virNWFilterDefParseXML(xmlXPathContextPtr ctxt) {
we allocate space for the result we expect to return:
if (VIR_ALLOC(ret) 0) {
virReportOOMError();
return NULL;
}
and later, ...
if (VIR_REALLOC_N(ret-filterEntries, ret-nentries+1) 0) {
VIR_FREE(entry);
virReportOOMError();
goto cleanup;
}
Hence, the lack of anything to free ret when this function returns
NULL constitutes a leak.
Here's the patch:
From 4c13990a15b33f03595d58b46b6e34e03bfffa65 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 12:05:53 +0200
Subject: [PATCH] virNWFilterDefParseXML: avoid leak on error paths
* src/conf/nwfilter_conf.c (virNWFilterDefParseXML): Also free ret
via cleanup.
---
src/conf/nwfilter_conf.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index ea73a33..fc6d461 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -1767,6 +1767,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) {
return ret;
cleanup:
+virNWFilterDefFree(ret);
VIR_FREE(chain);
VIR_FREE(uuid);
return NULL;
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] linuxNodeInfoCPUPopulate: avoid used-uninitialized via a test
Jim Meyering wrote:
See the comment:
From 1b200ba22d742e685de0b9853c8cd276df8e129f Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 11:58:32 +0200
Subject: [PATCH] linuxNodeInfoCPUPopulate: avoid used-uninitialized via a test
* tests/nodeinfotest.c (linuxTestCompareFiles): Don't use
nodeinfo-member uninitialized. linuxNodeInfoCPUPopulate requires
that some of its nodeinfo members (including threads) be initialized
upon input. The nodeinfotest.c program lacked the initialization,
while the only other use (nodeGetInfo) did perform it.
It's not trivial to move the initialization into the function,
since nodeGetInfo sets at least one member after clearing the
buffer but before calling linuxNodeInfoCPUPopulate.
---
tests/nodeinfotest.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index 9aeb459..cb92215 100644
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -40,6 +40,8 @@ static int linuxTestCompareFiles(const char *cpuinfofile,
const char *outputfile
cpuinfo = fopen(cpuinfofile, r);
if (!cpuinfo)
return -1;
+
+memset(nodeinfo, 0, sizeof(*nodeinfo));
if (linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo) 0) {
fclose(cpuinfo);
return -1;
That was wrong.
Unlike at the other call point, nodeinfo is not a pointer here,
and the above didn't even compile (can't deref a non-pointer).
Here's the correct patch:
From c11a7ae1e7a83d0539dc5d7831f1c12971ff2dbc Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 11:58:32 +0200
Subject: [PATCH] linuxNodeInfoCPUPopulate: avoid used-uninitialized via a test
* tests/nodeinfotest.c (linuxTestCompareFiles): Don't use
nodeinfo-member uninitialized. linuxNodeInfoCPUPopulate requires
that some of its nodeinfo members (including threads) be initialized
upon input. The nodeinfotest.c program lacked the initialization,
while the only other use (nodeGetInfo) did perform it.
It's not trivial to move the initialization into the function,
since nodeGetInfo sets at least one member after clearing the
buffer but before calling linuxNodeInfoCPUPopulate.
---
tests/nodeinfotest.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c
index 9aeb459..ff056b9 100644
--- a/tests/nodeinfotest.c
+++ b/tests/nodeinfotest.c
@@ -40,6 +40,8 @@ static int linuxTestCompareFiles(const char *cpuinfofile,
const char *outputfile
cpuinfo = fopen(cpuinfofile, r);
if (!cpuinfo)
return -1;
+
+memset(nodeinfo, 0, sizeof(nodeinfo));
if (linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo) 0) {
fclose(cpuinfo);
return -1;
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemuMonitorTextMigrate: avoid leak on OOM-error path
Fix a minor leak:
From 0fc88bfcae06346e871d2a4a89febb160bc0857e Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 18 May 2010 12:17:23 +0200
Subject: [PATCH] qemuMonitorTextMigrate: avoid leak on OOM-error path
* src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrate): Also
free safedest buffer when failing.
---
src/qemu/qemu_monitor_text.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index ae5d4d2..ec3d69d 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -1149,6 +1149,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon,
if (virBufferError(extra)) {
virBufferFreeAndReset(extra);
virReportOOMError();
+free(safedest);
return -1;
}
if (virAsprintf(cmd, migrate %s\%s\,
virBufferContentAndReset(extra), safedest) 0) {
--
1.7.1.250.g7d1e8
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tests: do not ignore virInitialize failure
Simple... From f5ee09ed08473478b3ea3135d51125fbf687e402 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Tue, 18 May 2010 12:32:39 +0200 Subject: [PATCH] tests: do not ignore virInitialize failure * tests/nodeinfotest.c (mymain): Do not ignore virInitialize failure. Most other callers of virInitialize test for failure. --- tests/nodeinfotest.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index ff056b9..d3c500d 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -106,7 +106,8 @@ mymain(int argc, char **argv) return(EXIT_FAILURE); } -virInitialize(); +if (virInitialize() 0) +return EXIT_FAILURE; for (i = 0 ; i ARRAY_CARDINALITY(nodeData); i++) if (virtTestRun(nodeData[i], 1, linuxTestNodeInfo, nodeData[i]) != 0) -- 1.7.1.250.g7d1e8 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] python module set-up ignores virInitialize failure
I've just fixed code in a test that ignored virInitialize failure.
Looking at all uses, I saw one other: in python/libvirt-override.c,
where the initialization function ignores virInitialize failure:
void
#ifndef __CYGWIN__
initlibvirtmod
#else
initcygvirtmod
#endif
(void)
{
static int initialized = 0;
if (initialized != 0)
return;
virInitialize();
/* initialize the python extension module */
Py_InitModule((char *)
#ifndef __CYGWIN__
libvirtmod
#else
cygvirtmod
#endif
, libvirtMethods);
initialized = 1;
}
Unfortunately, this function is public, so we can't change its signature.
Any suggestions?
For reference, here's the function definition. It shows that there
are many ways in which virInitialize can fail, including its many
registration functions:
/**
* virInitialize:
*
* Initialize the library. It's better to call this routine at startup
* in multithreaded applications to avoid potential race when initializing
* the library.
*
* Returns 0 in case of success, -1 in case of error
*/
int
virInitialize(void)
{
if (initialized)
return(0);
initialized = 1;
if (virThreadInitialize() 0 ||
virErrorInitialize() 0 ||
virRandomInitialize(time(NULL) ^ getpid()))
return -1;
gcry_control(GCRYCTL_SET_THREAD_CBS, virTLSThreadImpl);
gcry_check_version(NULL);
virLogSetFromEnv();
DEBUG0(register drivers);
#if HAVE_WINSOCK2_H
if (winsock_init () == -1) return -1;
#endif
if (!bindtextdomain(GETTEXT_PACKAGE, LOCALEBASEDIR))
return (-1);
/*
* Note that the order is important: the first ones have a higher
* priority when calling virConnectOpen.
*/
#ifdef WITH_DRIVER_MODULES
/* We don't care if any of these fail, because the whole point
* is to allow users to only install modules they want to use.
* If they try to open a connection for a module that
* is not loaded they'll get a suitable error at that point
*/
virDriverLoadModule(test);
virDriverLoadModule(xen);
virDriverLoadModule(openvz);
virDriverLoadModule(vbox);
virDriverLoadModule(esx);
virDriverLoadModule(xenapi);
virDriverLoadModule(remote);
#else
# ifdef WITH_TEST
if (testRegister() == -1) return -1;
# endif
# ifdef WITH_XEN
if (xenRegister () == -1) return -1;
# endif
# ifdef WITH_OPENVZ
if (openvzRegister() == -1) return -1;
# endif
# ifdef WITH_PHYP
if (phypRegister() == -1) return -1;
# endif
# ifdef WITH_VBOX
if (vboxRegister() == -1) return -1;
# endif
# ifdef WITH_ESX
if (esxRegister() == -1) return -1;
# endif
# ifdef WITH_XENAPI
if (xenapiRegister() == -1) return -1;
# endif
# ifdef WITH_REMOTE
if (remoteRegister () == -1) return -1;
# endif
#endif
return(0);
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
