3:23 a.m.
When libvirtd is run from a build directory without being installed, it
should not depend on files from a libvirt package installed in the
system. Currently, APIs defined in src/ don't know whether libvirtd
is being run from the build dir or the installed dir. The following
additions provide the functionality to do so:
virSetUninstalledDir
virGetUninstalledDir
Example usage (utility = lxc|iohelper):
char *path_tmp = virGetUninstalledDir();
if (path_tmp)
/* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */
else
/* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1:
Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6):
Add utility functions for storing uninstalled location
Use virGetUninstalledDir() in src/util/virfile.c
Use virGetUninstalledDir() in src/lxc/lxc_conf.c
Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
Use virGetUninstalledDir() in src/fdstream.c
Remove obsolete function virFDStreamSetIOHelper()
3:23 a.m.
New subject: [libvirt] [PATCHv2 1/6] Add utility functions for storing uninstalled location
src/libvirt_private.syms
*Add symbols
daemon/libvirtd.c
*Set uninstallDir when libvirtd is running uninstalled
from a build tree
src/util/virutil.c
*Introduce virSetUninstalledDir
*Introduce virGetUninstalledDir
---
daemon/libvirtd.c | 1 +
src/libvirt_private.syms | 2 ++
src/util/virutil.c | 27 +++++++++++++++++++++++++++
src/util/virutil.h | 3 +++
4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4179147..dc3da2a 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1165,6 +1165,7 @@ int main(int argc, char **argv) {
exit(EXIT_FAILURE);
}
*tmp = '\0';
+ virSetUninstalledDir(argv[0]);
char *driverdir;
if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", argv[0])
< 0 ||
virAsprintfQuiet(&cpumap, "%s/../../src/cpu/cpu_map.xml",
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2357f95..3549868 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1997,6 +1997,7 @@ virGetGroupList;
virGetGroupName;
virGetHostname;
virGetSelfLastChanged;
+virGetUninstalledDir;
virGetUnprivSGIOSysfsPath;
virGetUserCacheDirectory;
virGetUserConfigDirectory;
@@ -2025,6 +2026,7 @@ virSetInherit;
virSetNonBlock;
virSetUIDGID;
virSetUIDGIDWithCaps;
+virSetUninstalledDir;
virStrIsPrint;
virUpdateSelfLastChanged;
virValidateWWN;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 733cdff..47afbc4 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2204,3 +2204,30 @@ void virUpdateSelfLastChanged(const char *path)
selfLastChanged = sb.st_ctime;
}
}
+
+static char *uninstalledDir = NULL;
+/**
+ * virSetUninstalledDir:
+ * @path: directory containing an uninstalled binary, for use in locating
+ * other uninstalled files
+ *
+ * Set a pointer to the path which can be accessed by all other APIs using
+ * virGetUninstalledDir().
+ */
+void virSetUninstalledDir(char *path)
+{
+ ignore_value(VIR_STRDUP(uninstalledDir, path));
+}
+
+/**
+ * virGetUninstalledDir:
+ *
+ * If libvirtd (or other binary) is running uninstalled from a build tree,
+ * return the directory containing the binary. A NULL return implies that
+ * the binary has been installed and paths are relying on <configmake.h>
+ * locations.
+ */
+char *virGetUninstalledDir(void)
+{
+ return uninstalledDir;
+}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 1f2efd5..3e6ba47 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -202,4 +202,7 @@ bool virIsSUID(void);
time_t virGetSelfLastChanged(void);
void virUpdateSelfLastChanged(const char *path);
+void virSetUninstalledDir(char *path);
+char *virGetUninstalledDir(void);
+
#endif /* __VIR_UTIL_H__ */
--
1.7.1
5:11 a.m.
New subject: [libvirt] [PATCHv2 1/6] Add utility functions for storing uninstalled location
On Tue, Mar 25, 2014 at 01:53:11PM +0530, Nehal J Wani wrote:
src/libvirt_private.syms
*Add symbols
daemon/libvirtd.c
*Set uninstallDir when libvirtd is running uninstalled
from a build tree
src/util/virutil.c
*Introduce virSetUninstalledDir
*Introduce virGetUninstalledDir
---
daemon/libvirtd.c | 1 +
src/libvirt_private.syms | 2 ++
src/util/virutil.c | 27 +++++++++++++++++++++++++++
src/util/virutil.h | 3 +++
4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4179147..dc3da2a 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1165,6 +1165,7 @@ int main(int argc, char **argv) {
exit(EXIT_FAILURE);
}
*tmp = '\0';
+ virSetUninstalledDir(argv[0]);
argv[0] is not a directory - it is the path to the libvirtd
file. Either this is a mistake, or this is a very misleading
function name.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:17 a.m.
New subject: [libvirt] [PATCHv2 1/6] Add utility functions for storing uninstalled location
On Tue, Mar 25, 2014 at 10:11:14AM +0000, Daniel P. Berrange wrote:
On Tue, Mar 25, 2014 at 01:53:11PM +0530, Nehal J Wani wrote:
> src/libvirt_private.syms
> *Add symbols
>
> daemon/libvirtd.c
> *Set uninstallDir when libvirtd is running uninstalled
> from a build tree
>
> src/util/virutil.c
> *Introduce virSetUninstalledDir
> *Introduce virGetUninstalledDir
>
> ---
> daemon/libvirtd.c | 1 +
> src/libvirt_private.syms | 2 ++
> src/util/virutil.c | 27 +++++++++++++++++++++++++++
> src/util/virutil.h | 3 +++
> 4 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 4179147..dc3da2a 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1165,6 +1165,7 @@ int main(int argc, char **argv) {
> exit(EXIT_FAILURE);
> }
> *tmp = '\0';
> + virSetUninstalledDir(argv[0]);
argv[0] is not a directory - it is the path to the libvirtd
file. Either this is a mistake, or this is a very misleading
function name.
Eww, gross, I see we're modifying argv[0] in place, ignore my
comment above.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:21 p.m.
New subject: [libvirt] [PATCHv2 1/6] Add utility functions for storing uninstalled location
On 03/25/2014 02:23 AM, Nehal J Wani wrote:
src/libvirt_private.syms
*Add symbols
daemon/libvirtd.c
*Set uninstallDir when libvirtd is running uninstalled
from a build tree
src/util/virutil.c
*Introduce virSetUninstalledDir
*Introduce virGetUninstalledDir
---
daemon/libvirtd.c | 1 +
src/libvirt_private.syms | 2 ++
src/util/virutil.c | 27 +++++++++++++++++++++++++++
src/util/virutil.h | 3 +++
4 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4179147..dc3da2a 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1165,6 +1165,7 @@ int main(int argc, char **argv) {
exit(EXIT_FAILURE);
}
*tmp = '\0';
+ virSetUninstalledDir(argv[0]);
Given Dan's complaint about us abusing this variable, I'll squash in
something to make the naming more obvious.
+++ b/src/util/virutil.c
@@ -2204,3 +2204,30 @@ void virUpdateSelfLastChanged(const char *path)
selfLastChanged = sb.st_ctime;
}
}
+
+static char *uninstalledDir = NULL;
Static variables are guaranteed 0-initialization by the C standard;
explicitly assigning NULL makes some compilers actually bloat the
executable size because they stick the variable into .data instead of
.bss (gcc knows better, though).
+/**
Style - we've been using 2 blank lines between functions (not enforced,
but does make it visually easier to spot breaks when scrolling through
files).
{
ignore_value(VIR_STRDUP(uninstalledDir, path));
Memory leak if this gets called more than once (not that it does; we
call it 0 times when installed and exactly once when uninstalled) -
plugging it will make sure we don't get false positives from Coverity
and the like.
+/**
> + * virGetUninstalledDir:
> + *
> + * If libvirtd (or other binary) is running uninstalled from a build tree,
> + * return the directory containing the binary. A NULL return implies that
> + * the binary has been installed and paths are relying on <configmake.h>
> + * locations.
> + */
> +char *virGetUninstalledDir(void)
Needs a const.
Potential ACK. Here's what I'll squash in, and I'll push the result
after reviewing the rest of the series if I don't spot anything else major.
Hmm, in re-reading things, I noticed that the one place you are calling
the new set function, you are passing in /path/to/builddir/daemon/.libs/
as the directory. I think it might work better to pass in
/path/to/builddir, and teach all clients to specify paths directly
relative to builddir, than having to do things relative to
/path/to/builddir/daemon/.libs/../..; I'll have to see how you actually
use the directory in later patches.
diff --git i/daemon/libvirtd.c w/daemon/libvirtd.c
index f0faf1a..0754fb5 100644
--- i/daemon/libvirtd.c
+++ w/daemon/libvirtd.c
@@ -1158,23 +1158,26 @@ int main(int argc, char **argv) {
if (strstr(argv[0], "lt-libvirtd") ||
strstr(argv[0], "/daemon/.libs/libvirtd")) {
+ char *dir;
char *tmp = strrchr(argv[0], '/');
char *cpumap;
- if (!tmp) {
- fprintf(stderr, _("%s: cannot identify driver
directory\n"), argv[0]);
+ char *driverdir;
+
+ if (!tmp || VIR_STRNDUP(dir, argv[0], tmp - argv[0]) < 0) {
+ fprintf(stderr, _("%s: cannot identify driver directory\n"),
+ argv[0]);
exit(EXIT_FAILURE);
}
- *tmp = '\0';
- virSetUninstalledDir(argv[0]);
- char *driverdir;
- if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", argv[0])
< 0 ||
+ virSetUninstalledDir(dir);
+ if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", dir) < 0
||
virAsprintfQuiet(&cpumap, "%s/../../src/cpu/cpu_map.xml",
- argv[0]) < 0) {
+ dir) < 0) {
fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
exit(EXIT_FAILURE);
}
if (access(driverdir, R_OK) < 0) {
- fprintf(stderr, _("%s: expected driver directory '%s' is
missing\n"),
+ fprintf(stderr,
+ _("%s: expected driver directory '%s' is
missing\n"),
argv[0], driverdir);
exit(EXIT_FAILURE);
}
@@ -1184,7 +1187,7 @@ int main(int argc, char **argv) {
#endif
cpuMapOverride(cpumap);
VIR_FREE(cpumap);
- *tmp = '/';
+ VIR_FREE(dir);
/* Must not free 'driverdir' - it is still used */
}
diff --git i/src/util/virutil.c w/src/util/virutil.c
index 244b9e9..dabd4f8 100644
--- i/src/util/virutil.c
+++ w/src/util/virutil.c
@@ -2205,7 +2205,9 @@ void virUpdateSelfLastChanged(const char *path)
}
}
-static char *uninstalledDir = NULL;
+
+static char *uninstalledDir;
+
/**
* virSetUninstalledDir:
* @path: directory containing an uninstalled binary, for use in locating
@@ -2214,8 +2216,9 @@ static char *uninstalledDir = NULL;
* Set a pointer to the path which can be accessed by all other APIs using
* virGetUninstalledDir().
*/
-void virSetUninstalledDir(char *path)
+void virSetUninstalledDir(const char *path)
{
+ VIR_FREE(uninstalledDir);
ignore_value(VIR_STRDUP(uninstalledDir, path));
}
@@ -2227,7 +2230,7 @@ void virSetUninstalledDir(char *path)
* the binary has been installed and paths are relying on <configmake.h>
* locations.
*/
-char *virGetUninstalledDir(void)
+const char *virGetUninstalledDir(void)
{
return uninstalledDir;
}
diff --git i/src/util/virutil.h w/src/util/virutil.h
index 3e6ba47..437167a 100644
--- i/src/util/virutil.h
+++ w/src/util/virutil.h
@@ -202,7 +202,7 @@ bool virIsSUID(void);
time_t virGetSelfLastChanged(void);
void virUpdateSelfLastChanged(const char *path);
-void virSetUninstalledDir(char *path);
-char *virGetUninstalledDir(void);
+void virSetUninstalledDir(const char *path);
+const char *virGetUninstalledDir(void);
#endif /* __VIR_UTIL_H__ */
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:31 a.m.
New subject: [libvirt] [PATCHv2 1/6] Add utility functions for storing uninstalled location
On Tue, Mar 25, 2014 at 01:53:11PM +0530, Nehal J Wani wrote:
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 4179147..dc3da2a 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1165,6 +1165,7 @@ int main(int argc, char **argv) {
exit(EXIT_FAILURE);
}
*tmp = '\0';
+ virSetUninstalledDir(argv[0]);
Should check for failure
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 733cdff..47afbc4 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2204,3 +2204,30 @@ void virUpdateSelfLastChanged(const char *path)
selfLastChanged = sb.st_ctime;
}
}
+
+static char *uninstalledDir = NULL;
+/**
+ * virSetUninstalledDir:
+ * @path: directory containing an uninstalled binary, for use in locating
+ * other uninstalled files
+ *
+ * Set a pointer to the path which can be accessed by all other APIs using
+ * virGetUninstalledDir().
+ */
+void virSetUninstalledDir(char *path)
Arg shoud be 'const char *'
+{
+ ignore_value(VIR_STRDUP(uninstalledDir, path));
+}
We shouldn't ignore OOM - we should propagate it.
+
+/**
+ * virGetUninstalledDir:
+ *
+ * If libvirtd (or other binary) is running uninstalled from a build tree,
+ * return the directory containing the binary. A NULL return implies that
+ * the binary has been installed and paths are relying on <configmake.h>
+ * locations.
+ */
+char *virGetUninstalledDir(void)
Return value should be const *
ACK, but I'm squashing in
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index f0faf1a..021fdc7 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1165,7 +1165,10 @@ int main(int argc, char **argv) {
exit(EXIT_FAILURE);
}
*tmp = '\0';
- virSetUninstalledDir(argv[0]);
+ if (virSetUninstalledDir(argv[0]) < 0) {
+ fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
+ exit(EXIT_FAILURE);
+ }
char *driverdir;
if (virAsprintfQuiet(&driverdir, "%s/../../src/.libs", argv[0])
< 0 ||
virAsprintfQuiet(&cpumap, "%s/../../src/cpu/cpu_map.xml",
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 244b9e9..db94a56 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -2213,10 +2213,13 @@ static char *uninstalledDir = NULL;
*
* Set a pointer to the path which can be accessed by all other APIs using
* virGetUninstalledDir().
+ *
+ * Returns -1 on error, 0 on success
*/
-void virSetUninstalledDir(char *path)
+int virSetUninstalledDir(const char *path)
{
- ignore_value(VIR_STRDUP(uninstalledDir, path));
+ VIR_FREE(uninstalledDir);
+ return VIR_STRDUP(uninstalledDir, path);
}
/**
@@ -2227,7 +2230,7 @@ void virSetUninstalledDir(char *path)
* the binary has been installed and paths are relying on <configmake.h>
* locations.
*/
-char *virGetUninstalledDir(void)
+const char *virGetUninstalledDir(void)
{
return uninstalledDir;
}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 3e6ba47..13db8c8 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -202,7 +202,7 @@ bool virIsSUID(void);
time_t virGetSelfLastChanged(void);
void virUpdateSelfLastChanged(const char *path);
-void virSetUninstalledDir(char *path);
-char *virGetUninstalledDir(void);
+int virSetUninstalledDir(const char *path) ATTRIBUTE_RETURN_CHECK;
+const char *virGetUninstalledDir(void);
#endif /* __VIR_UTIL_H__ */
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:23 a.m.
New subject: [libvirt] [PATCHv2 2/6] Use virGetUninstalledDir() in src/util/virfile.c
src/util/virfile.c
*Check if libvirtd is running uninstalled from a build tree and change
iohelper_path accordingly
---
src/util/virfile.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index fefc3fb..813ad86 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -193,6 +193,8 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags)
bool output = false;
int pipefd[2] = { -1, -1 };
int mode = -1;
+ char *uninstalledDir = NULL;
+ char *iohelper_path = NULL;
if (!flags) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -236,8 +238,20 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags)
goto error;
}
- ret->cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper",
- name, "0", NULL);
+ uninstalledDir = virGetUninstalledDir();
+
+ if (uninstalledDir && virAsprintf(&iohelper_path,
+ "%s/../../src/libvirt_iohelper",
+ uninstalledDir) < 0)
+ goto error;
+ else if (virAsprintf(&iohelper_path,
+ LIBEXECDIR "/libvirt_iohelper") < 0)
+ goto error;
+
+ ret->cmd = virCommandNewArgList(iohelper_path, name, "0", NULL);
+
+ VIR_FREE(iohelper_path);
+
if (output) {
virCommandSetInputFD(ret->cmd, pipefd[0]);
virCommandSetOutputFD(ret->cmd, fd);
@@ -268,6 +282,7 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags)
return ret;
error:
+ VIR_FREE(iohelper_path);
VIR_FORCE_CLOSE(pipefd[0]);
VIR_FORCE_CLOSE(pipefd[1]);
virFileWrapperFdFree(ret);
--
1.7.1
4:17 p.m.
New subject: [libvirt] [PATCHv2 2/6] Use virGetUninstalledDir() in src/util/virfile.c
On 03/25/2014 02:23 AM, Nehal J Wani wrote:
src/util/virfile.c
*Check if libvirtd is running uninstalled from a build tree and change
iohelper_path accordingly
---
src/util/virfile.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
That's a rather large diffstat for the convenience of running
uninstalled. I still think we need this series; but it's making me
wonder if we are putting too much repetitive burden on individual
callers, which should instead be factored into the common helper routine.
int mode = -1;
+ char *uninstalledDir = NULL;
Missing a const (more obvious once the .h in patch 1/6 documents const
in the function declaration).
+ uninstalledDir = virGetUninstalledDir();
+
+ if (uninstalledDir && virAsprintf(&iohelper_path,
+ "%s/../../src/libvirt_iohelper",
+ uninstalledDir) < 0)
Yep, this confirms my suspicion that I very much dislike patch 1/6
supplying '/path/to/build/daemon/.libs' as the uninstalled dir; it's
forcing the caller in a completely different file to have to guess that
must supply a leading '../../'. I really do think it's better to have
virSetUninstalledDir store the location of ${abs_builddir}, not
${abs_builddir}/daemon/.libs.
The patch in isolation is fine, but I'm more worried about the bigger
picture. That is, the high-level view of what we are doing here is:
find me the correct 'libvirt_iohelper' binary, favoring ${builddir}/src
if running in-tree, and LIBEXECDIR otherwise.
In patch 3/6, we want:
find me the correct 'libvirt_lxc' binary, favoring ${builddir}/src if
running in-tree, and LIBEXECDIR otherwise.
In daemon/libvirtd.c (touched in 1/6), we have pre-existing uses of
virAsprintf, feeding into src/cpu/cpu_map.c, where we are in reality
trying to express:
find me the correct 'cpu_map.xml', favoring ${builddir}/src/cpu/ if
running in-tree, and PKGDATADIR otherwise.
[In fact, for cpu_map.xml, it's even worse - we did some makefile
hackery to guarantee that in a VPATH build,
${builddir}/src/cpu/cpu_map.xml is a symlink to
${srcdir}/src/cpu/cpu_map.xml]
I'm sensing a common pattern - I think a more useful interface might be:
binary = virResourceLocate("libvirt_iohelper", LIBEXECDIR, "src");
virCommandNew(binary);
VIR_FREE(binary);
mapfile = virResourceLocate("cpu_map.xml", PKGDATADIR, "src/cpu");
xmlParseFile(mapfile);
VIR_FREE(mapfile);
I think that having virResourceLocate do the magic of deciding if we are
running uninstalled, and do the virAsprintf, will make each call-site
simpler. Furthermore, in the case of cpu_map.xml, we might even be able
to figure out the srcdir vs. builddir tricks of VPATH builds without
having to do quite so much makefile hackery.
I also think that while your series is a good start, it feels incomplete
until we have factored ALL of the disparate override calls in
daemon/libvirtd.c to use the common theme of determining an in-tree
resource when running uninstalled, instead of having 3 or 4 different
approaches of which functions we call to register the fact that we are
running in-tree.
I'll see what I can do to help.
Meanwhile, please don't let the slow review of this series stall you
from submitting further patches. It's okay to submit patches that have
prerequisites that still need review, and in fact, doing that sometimes
gets the missing reviews supplied faster once people realize the missing
reviews are holding up further development.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:37 a.m.
New subject: [libvirt] [PATCHv2 2/6] Use virGetUninstalledDir() in src/util/virfile.c
On Tue, Mar 25, 2014 at 01:53:12PM +0530, Nehal J Wani wrote:
src/util/virfile.c
*Check if libvirtd is running uninstalled from a build tree and change
iohelper_path accordingly
---
src/util/virfile.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index fefc3fb..813ad86 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -193,6 +193,8 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags)
bool output = false;
int pipefd[2] = { -1, -1 };
int mode = -1;
+ char *uninstalledDir = NULL;
Should be 'const char *' since we don't own this pointer
ACK will fix when pushing
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:23 a.m.
New subject: [libvirt] [PATCHv2 3/6] Use virGetUninstalledDir() in src/lxc/lxc_conf.c
src/lxc/lxc_conf.c:
*Check if libvirtd is running uninstalled from a build tree and change
lxc_path accordingly
---
src/lxc/lxc_conf.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
index d4432cf..ad8f78a 100644
--- a/src/lxc/lxc_conf.c
+++ b/src/lxc/lxc_conf.c
@@ -66,6 +66,8 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver)
virCapsPtr caps;
virCapsGuestPtr guest;
virArch altArch;
+ char *uninstalledDir = NULL;
+ char *lxc_path = NULL;
if ((caps = virCapabilitiesNew(virArchFromHost(),
0, 0)) == NULL)
@@ -89,10 +91,20 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver)
goto error;
}
+ uninstalledDir = virGetUninstalledDir();
+
+ if (uninstalledDir && virAsprintf(&lxc_path,
+ "%s/../../src/libvirt_lxc",
+ uninstalledDir) < 0)
+ goto error;
+ else if (virAsprintf(&lxc_path,
+ LIBEXECDIR "/libvirt_lxc") < 0)
+ goto error;
+
if ((guest = virCapabilitiesAddGuest(caps,
"exe",
caps->host.arch,
- LIBEXECDIR "/libvirt_lxc",
+ lxc_path,
NULL,
0,
NULL)) == NULL)
@@ -111,7 +123,7 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver)
if ((guest = virCapabilitiesAddGuest(caps,
"exe",
altArch,
- LIBEXECDIR "/libvirt_lxc",
+ lxc_path,
NULL,
0,
NULL)) == NULL)
@@ -126,6 +138,8 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver)
goto error;
}
+ VIR_FREE(lxc_path);
+
if (driver) {
/* Security driver data */
const char *doi, *model, *label, *type;
@@ -158,6 +172,7 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver)
return caps;
error:
+ VIR_FREE(lxc_path);
virObjectUnref(caps);
return NULL;
}
--
1.7.1
8:48 a.m.
New subject: [libvirt] [PATCHv2 3/6] Use virGetUninstalledDir() in src/lxc/lxc_conf.c
On Tue, Mar 25, 2014 at 01:53:13PM +0530, Nehal J Wani wrote:
src/lxc/lxc_conf.c:
*Check if libvirtd is running uninstalled from a build tree and change
lxc_path accordingly
---
src/lxc/lxc_conf.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
index d4432cf..ad8f78a 100644
--- a/src/lxc/lxc_conf.c
+++ b/src/lxc/lxc_conf.c
@@ -66,6 +66,8 @@ virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver)
virCapsPtr caps;
virCapsGuestPtr guest;
virArch altArch;
+ char *uninstalledDir = NULL;
Again should have been const.
ACK will fix that myself
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:23 a.m.
New subject: [libvirt] [PATCHv2 4/6] Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
src/storage/storage_backend_disk.c:
*Check if libvirtd is running uninstalled from a build tree and change
parthelper_path accordingly
---
src/storage/storage_backend_disk.c | 28 ++++++++++++++++++++++++++--
1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 81201fd..bb29d39 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -261,7 +261,18 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
* - normal metadata 100027630080 100030242304 2612736
*
*/
- virCommandPtr cmd = virCommandNewArgList(PARTHELPER,
+
+ char *uninstalledDir = virGetUninstalledDir();
+ char *parthelper_path = NULL;
+
+ if (uninstalledDir && virAsprintf(&parthelper_path,
+ "%s/../../src/libvirt_parthelper",
+ uninstalledDir) < 0)
+ return -1;
+ else if (virAsprintf(&parthelper_path, PARTHELPER) < 0)
+ return -1;
+
+ virCommandPtr cmd = virCommandNewArgList(parthelper_path,
pool->def->source.devices[0].path,
NULL);
struct virStorageBackendDiskPoolVolData cbdata = {
@@ -277,6 +288,7 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
virStorageBackendDiskMakeVol,
&cbdata);
virCommandFree(cmd);
+ VIR_FREE(parthelper_path);
return ret;
}
@@ -301,7 +313,18 @@ virStorageBackendDiskMakePoolGeometry(size_t ntok ATTRIBUTE_UNUSED,
static int
virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
{
- virCommandPtr cmd = virCommandNewArgList(PARTHELPER,
+ char *uninstalledDir = virGetUninstalledDir();
+ char *parthelper_path = NULL;
+
+ if (uninstalledDir && virAsprintf(&parthelper_path,
+ "%s/../../src/libvirt_parthelper",
+ uninstalledDir) < 0)
+ return -1;
+ else if (virAsprintf(&parthelper_path, PARTHELPER) < 0)
+ return -1;
+
+
+ virCommandPtr cmd = virCommandNewArgList(parthelper_path,
pool->def->source.devices[0].path,
"-g",
NULL);
@@ -312,6 +335,7 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
virStorageBackendDiskMakePoolGeometry,
pool);
virCommandFree(cmd);
+ VIR_FREE(parthelper_path);
return ret;
}
--
1.7.1
8:51 a.m.
New subject: [libvirt] [PATCHv2 4/6] Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
On Tue, Mar 25, 2014 at 01:53:14PM +0530, Nehal J Wani wrote:
src/storage/storage_backend_disk.c:
*Check if libvirtd is running uninstalled from a build tree and change
parthelper_path accordingly
---
src/storage/storage_backend_disk.c | 28 ++++++++++++++++++++++++++--
1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 81201fd..bb29d39 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -261,7 +261,18 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
* - normal metadata 100027630080 100030242304 2612736
*
*/
- virCommandPtr cmd = virCommandNewArgList(PARTHELPER,
+
+ char *uninstalledDir = virGetUninstalledDir();
+ char *parthelper_path = NULL;
+
+ if (uninstalledDir && virAsprintf(&parthelper_path,
+ "%s/../../src/libvirt_parthelper",
+ uninstalledDir) < 0)
+ return -1;
+ else if (virAsprintf(&parthelper_path, PARTHELPER) < 0)
+ return -1;
+
+ virCommandPtr cmd = virCommandNewArgList(parthelper_path,
Hmm, it getting rather ugly & tedious to repeat this logic every
place we need it.
I think we need another helper API really
virFileFindBinary(const char *srcdir,
const char *bindir,
const char *binaryname);
which we'd call thus
path = virFileFindBinary("../../src", LIBEXECDIR,
"libvirt_parthelper")
I think I will re-write this series todo that and re-post
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:38 a.m.
New subject: [libvirt] [PATCHv2 4/6] Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
On 04/24/2014 07:51 AM, Daniel P. Berrange wrote:
On Tue, Mar 25, 2014 at 01:53:14PM +0530, Nehal J Wani wrote:
> src/storage/storage_backend_disk.c:
> *Check if libvirtd is running uninstalled from a build tree and change
> parthelper_path accordingly
>
Hmm, it getting rather ugly & tedious to repeat this logic every
place we need it.
I came to the same conclusion yesterday :)
I think we need another helper API really
virFileFindBinary(const char *srcdir,
const char *bindir,
const char *binaryname);
Slightly different signature than my proposal - please read
https://www.redhat.com/archives/libvir-list/2014-April/msg00902.html
before doing too much duplicate work.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:23 a.m.
New subject: [libvirt] [PATCHv2 5/6] Use virGetUninstalledDir() in src/fdstream.c
src/storage/storage_backend_disk.c:
*Check if libvirtd is running uninstalled from a build tree and change
parthelper_path accordingly
---
src/fdstream.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c
index d0435c7..031d9f5 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -593,6 +593,8 @@ virFDStreamOpenFileInternal(virStreamPtr st,
struct stat sb;
virCommandPtr cmd = NULL;
int errfd = -1;
+ char *uninstalledDir = NULL;
+ char *iohelper_path = NULL;
VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o",
st, path, oflags, offset, length, mode);
@@ -648,9 +650,22 @@ virFDStreamOpenFileInternal(virStreamPtr st,
goto error;
}
+ uninstalledDir = virGetUninstalledDir();
+
+ if (uninstalledDir && virAsprintf(&iohelper_path,
+ "%s/../../src/libvirt_iohelper",
+ uninstalledDir) < 0)
+ goto error;
+ else if (virAsprintf(&iohelper_path,
+ LIBEXECDIR "/libvirt_iohelper") < 0)
+ goto error;
+
cmd = virCommandNewArgList(iohelper_path,
path,
NULL);
+
+ VIR_FREE(iohelper_path);
+
virCommandAddArgFormat(cmd, "%llu", length);
virCommandPassFD(cmd, fd,
VIR_COMMAND_PASS_FD_CLOSE_PARENT);
@@ -683,6 +698,7 @@ error:
VIR_FORCE_CLOSE(fd);
VIR_FORCE_CLOSE(childfd);
VIR_FORCE_CLOSE(errfd);
+ VIR_FREE(iohelper_path);
if (oflags & O_CREAT)
unlink(path);
return -1;
--
1.7.1
3:23 a.m.
New subject: [libvirt] [PATCHv2 6/6] Remove obsolete function virFDStreamSetIOHelper()
Now that we have virSetUninstalledDir() and virGetUninstalledDir(), we
no longer need virFDStreamSetIOHelper().
---
src/fdstream.c | 11 -----------
src/fdstream.h | 2 --
src/libvirt_private.syms | 1 -
tests/fdstreamtest.c | 3 ---
4 files changed, 0 insertions(+), 17 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c
index 031d9f5..dbe2781 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -78,17 +78,6 @@ struct virFDStreamData {
};
-static const char *iohelper_path = LIBEXECDIR "/libvirt_iohelper";
-
-void virFDStreamSetIOHelper(const char *path)
-{
- if (path == NULL)
- iohelper_path = LIBEXECDIR "/libvirt_iohelper";
- else
- iohelper_path = path;
-}
-
-
static int virFDStreamRemoveCallback(virStreamPtr stream)
{
struct virFDStreamData *fdst = stream->privateData;
diff --git a/src/fdstream.h b/src/fdstream.h
index 9c7295d..6292cd5 100644
--- a/src/fdstream.h
+++ b/src/fdstream.h
@@ -34,8 +34,6 @@ typedef void (*virFDStreamInternalCloseCbFreeOpaque)(void *opaque);
/* Only for use by test suite */
-void virFDStreamSetIOHelper(const char *path);
-
int virFDStreamOpen(virStreamPtr st,
int fd);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3549868..6ac54ea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -809,7 +809,6 @@ virFDStreamCreateFile;
virFDStreamOpen;
virFDStreamOpenFile;
virFDStreamOpenPTY;
-virFDStreamSetIOHelper;
# libvirt_internal.h
diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c
index 92e7add..f1ef42a 100644
--- a/tests/fdstreamtest.c
+++ b/tests/fdstreamtest.c
@@ -321,9 +321,6 @@ mymain(void)
{
char scratchdir[] = SCRATCHDIRTEMPLATE;
int ret = 0;
- const char *iohelper = abs_builddir "/../src/libvirt_iohelper";
-
- virFDStreamSetIOHelper(iohelper);
if (!mkdtemp(scratchdir)) {
virFilePrintf(stderr, "Cannot create fakesysfsdir");
--
1.7.1
3:55 p.m.
New subject: [libvirt] [PATCHv2 6/6] Remove obsolete function virFDStreamSetIOHelper()
On 03/25/2014 02:23 AM, Nehal J Wani wrote:
Now that we have virSetUninstalledDir() and virGetUninstalledDir(),
we
no longer need virFDStreamSetIOHelper().
---
-static const char *iohelper_path = LIBEXECDIR
"/libvirt_iohelper";
-
-void virFDStreamSetIOHelper(const char *path)
-{
- if (path == NULL)
- iohelper_path = LIBEXECDIR "/libvirt_iohelper";
- else
- iohelper_path = path;
-}
Problem. Just because we have virGetUninstalldDir() doesn't mean we are
using that, unless you ALSO fix the testsuite to call
virSetUninstalledDir(). As written, your series only calls
virSetUninstalledDir() in daemon/libvirtd.c,...
+++ b/tests/fdstreamtest.c
@@ -321,9 +321,6 @@ mymain(void)
{
char scratchdir[] = SCRATCHDIRTEMPLATE;
int ret = 0;
- const char *iohelper = abs_builddir "/../src/libvirt_iohelper";
-
- virFDStreamSetIOHelper(iohelper);
...but fdstreamtest.c is not using daemon/libvirtd.c, which means you
just broke this file into running the pre-installed version instead of
the in-tree version. For this series to work, you need to make
testutils.c call virSetUninstalledDir on behalf of all test programs.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:10 p.m.
On Tue, Mar 25, 2014 at 1:53 PM, Nehal J Wani <nehaljw.kkd1(a)gmail.com> wrote:
When libvirtd is run from a build directory without being installed,
it
should not depend on files from a libvirt package installed in the
system. Currently, APIs defined in src/ don't know whether libvirtd
is being run from the build dir or the installed dir. The following
additions provide the functionality to do so:
virSetUninstalledDir
virGetUninstalledDir
Example usage (utility = lxc|iohelper):
char *path_tmp = virGetUninstalledDir();
if (path_tmp)
/* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */
else
/* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1:
Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6):
Add utility functions for storing uninstalled location
Use virGetUninstalledDir() in src/util/virfile.c
Use virGetUninstalledDir() in src/lxc/lxc_conf.c
Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
Use virGetUninstalledDir() in src/fdstream.c
Remove obsolete function virFDStreamSetIOHelper()
Ping!
--
Nehal J Wani
12:16 p.m.
On Tue, Mar 25, 2014 at 1:53 PM, Nehal J Wani <nehaljw.kkd1(a)gmail.com> wrote:
When libvirtd is run from a build directory without being installed,
it
should not depend on files from a libvirt package installed in the
system. Currently, APIs defined in src/ don't know whether libvirtd
is being run from the build dir or the installed dir. The following
additions provide the functionality to do so:
virSetUninstalledDir
virGetUninstalledDir
Example usage (utility = lxc|iohelper):
char *path_tmp = virGetUninstalledDir();
if (path_tmp)
/* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */
else
/* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1:
Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6):
Add utility functions for storing uninstalled location
Use virGetUninstalledDir() in src/util/virfile.c
Use virGetUninstalledDir() in src/lxc/lxc_conf.c
Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
Use virGetUninstalledDir() in src/fdstream.c
Remove obsolete function virFDStreamSetIOHelper()
Ping?
--
Nehal J Wani
1:29 p.m.
Ping!
Once this gets through, the custom leases API will get through, and
then the virNetworkGetDHCPLeases and virNetworkGetDHCPLeasesForMAC
APIs will be introduced.
On Tue, Mar 25, 2014 at 1:53 PM, Nehal J Wani <nehaljw.kkd1(a)gmail.com> wrote:
When libvirtd is run from a build directory without being installed,
it
should not depend on files from a libvirt package installed in the
system. Currently, APIs defined in src/ don't know whether libvirtd
is being run from the build dir or the installed dir. The following
additions provide the functionality to do so:
virSetUninstalledDir
virGetUninstalledDir
Example usage (utility = lxc|iohelper):
char *path_tmp = virGetUninstalledDir();
if (path_tmp)
/* do stuff with ("%s/../../src/libvirt_<utility>", path_tmp) */
else
/* do stuff with (LIBEXECDIR "/libvirt_<utility>") */
v1:
Refer: https://www.redhat.com/archives/libvir-list/2014-March/msg01427.html
Nehal J Wani (6):
Add utility functions for storing uninstalled location
Use virGetUninstalledDir() in src/util/virfile.c
Use virGetUninstalledDir() in src/lxc/lxc_conf.c
Use virGetUninstalledDir() in src/storage/storage_backend_disk.c
Use virGetUninstalledDir() in src/fdstream.c
Remove obsolete function virFDStreamSetIOHelper()
--
Nehal J Wani
3839
days inactive
3869
days old
19 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Nehal J Wani