Unless overridden, libdevmapper logs all messages to stderr.
Therefore if something goes wrong in storage_backend_mpath.c or
parthelper.c we don't honour user set logging targets.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virlog.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c
index dd927f0ba7..51f2d341db 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -40,6 +40,9 @@
#if HAVE_SYS_UN_H
# include <sys/un.h>
#endif
+#if WITH_DEVMAPPER
+# include <libdevmapper.h>
+#endif
#include "virerror.h"
#include "virlog.h"
@@ -258,6 +261,20 @@ virLogPriorityString(virLogPriority lvl)
}
+#ifdef WITH_DEVMAPPER
+static void
+virLogDM(int level ATTRIBUTE_UNUSED,
+ const char *file ATTRIBUTE_UNUSED,
+ int line ATTRIBUTE_UNUSED,
+ int dm_errno ATTRIBUTE_UNUSED,
+ const char *fmt ATTRIBUTE_UNUSED,
+ ...)
+{
+ return;
+}
+#endif
+
+
static int
virLogOnceInit(void)
{
@@ -289,6 +306,12 @@ virLogOnceInit(void)
NUL_TERMINATE(virLogHostname);
}
+#ifdef WITH_DEVMAPPER
+ /* Ideally, we would not need this. But libdevmapper prints
+ * error messages to stderr by default. Sad but true. */
+ dm_log_with_errno_init(virLogDM);
+#endif
+
virLogUnlock();
return 0;
}
--
2.16.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote: > Unless overridden, libdevmapper logs all messages to stderr. > Therefore if something goes wrong in storage_backend_mpath.c or > parthelper.c we don't honour user set logging targets. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > src/util/virlog.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) I think this should be done in the function which registers the mpath storage backend. For now it should work as is, but given that we are going to split the daemons this code will not really be necessary in other places. And it really does not have to do much with our logging code. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 26, 2018 at 09:49:08 +0200, Peter Krempa wrote: > On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote: > > Unless overridden, libdevmapper logs all messages to stderr. > > Therefore if something goes wrong in storage_backend_mpath.c or > > parthelper.c we don't honour user set logging targets. 'parthelper.c' does not use our logging code and also does not use any libdevmapper.h functions. According to my experiment the header file include can be dropped. > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > > --- > > src/util/virlog.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > I think this should be done in the function which registers the mpath > storage backend. For now it should work as is, but given that we are > going to split the daemons this code will not really be necessary in > other places. So looking at the new code you've added in this series, it still might be desirable to move all the libdevmapper use into the storage driver and keep it there, so users wanting to use multipath would need to install the corresponding storage driver. Given that moving it there might be impractical, I'll suggest that you move it to a separate util file and add virOnce infrastructure to set the error helpers. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/26/2018 10:44 AM, Peter Krempa wrote: > On Mon, Mar 26, 2018 at 09:49:08 +0200, Peter Krempa wrote: >> On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote: >>> Unless overridden, libdevmapper logs all messages to stderr. >>> Therefore if something goes wrong in storage_backend_mpath.c or >>> parthelper.c we don't honour user set logging targets. > > 'parthelper.c' does not use our logging code and also does not use any > libdevmapper.h functions. According to my experiment the header file > include can be dropped. > >>> >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >>> --- >>> src/util/virlog.c | 23 +++++++++++++++++++++++ >>> 1 file changed, 23 insertions(+) >> >> I think this should be done in the function which registers the mpath >> storage backend. For now it should work as is, but given that we are >> going to split the daemons this code will not really be necessary in >> other places. > > So looking at the new code you've added in this series, it still might > be desirable to move all the libdevmapper use into the storage driver > and keep it there, so users wanting to use multipath would need to > install the corresponding storage driver. > > Given that moving it there might be impractical, I'll suggest that you > move it to a separate util file and add virOnce infrastructure to set > the error helpers. > Okay, fair enough. I'll wait for review on the rest of the patches before I post v2. Thanks, Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/26/2018 09:49 AM, Peter Krempa wrote: > On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote: >> Unless overridden, libdevmapper logs all messages to stderr. >> Therefore if something goes wrong in storage_backend_mpath.c or >> parthelper.c we don't honour user set logging targets. >> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >> --- >> src/util/virlog.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) > > I think this should be done in the function which registers the mpath > storage backend. For now it should work as is, but given that we are > going to split the daemons this code will not really be necessary in > other places. > > And it really does not have to do much with our logging code. > Well, in the next patch I'm introducing virFileGetMPathTargets() which uses dm_* a lot. Now, since the function lives in src/util/ it can be used by many drivers. Do we expect each one of them to provide dummy callback on their own? I don't think so. I agree this is not a nice patch, but I don't really see any better place for it. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 26, 2018 at 10:16:54 +0200, Michal Privoznik wrote: > On 03/26/2018 09:49 AM, Peter Krempa wrote: > > On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote: > >> Unless overridden, libdevmapper logs all messages to stderr. > >> Therefore if something goes wrong in storage_backend_mpath.c or > >> parthelper.c we don't honour user set logging targets. > >> > >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > >> --- > >> src/util/virlog.c | 23 +++++++++++++++++++++++ > >> 1 file changed, 23 insertions(+) > > > > I think this should be done in the function which registers the mpath > > storage backend. For now it should work as is, but given that we are > > going to split the daemons this code will not really be necessary in > > other places. > > > > And it really does not have to do much with our logging code. > > > > Well, in the next patch I'm introducing virFileGetMPathTargets() which > uses dm_* a lot. Now, since the function lives in src/util/ it can be > used by many drivers. Do we expect each one of them to provide dummy > callback on their own? I don't think so. I agree this is not a nice > patch, but I don't really see any better place for it. I've written another response to my original reply which explains a bit more. Basically, the code in the next patch should be put in a new file along with a virOnce function to silence the logs if you don't want to move it into the storage driver directly. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.