* [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly @ 2019-08-21 8:56 Eryu Guan 2019-08-21 15:04 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 11+ messages in thread From: Eryu Guan @ 2019-08-21 8:56 UTC (permalink / raw) To: virtio-fs Introduce a file which contains the target log level, and current_log_level will be reloaded to the target log level on SIGHUP. The file is set to "/run/virtiofsd/log_level.<pid>". Due to sandboxing in virtiofsd, this file can only be read/write via /proc/<pid>/fd/<fd> for now. Example usage: # view current log level cat /proc/<pid>/fd/<fd> # change log level to debug echo debug > /proc/<pid>/fd/<fd> kill -HUP <pid> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> --- contrib/virtiofsd/fuse_log.c | 144 +++++++++++++++++++++++++++++++++++++ contrib/virtiofsd/fuse_log.h | 4 ++ contrib/virtiofsd/fuse_signals.c | 4 +- contrib/virtiofsd/passthrough_ll.c | 4 ++ 4 files changed, 154 insertions(+), 2 deletions(-) diff --git a/contrib/virtiofsd/fuse_log.c b/contrib/virtiofsd/fuse_log.c index d54b64099a2b..adfcbb84bfb3 100644 --- a/contrib/virtiofsd/fuse_log.c +++ b/contrib/virtiofsd/fuse_log.c @@ -6,12 +6,24 @@ * See the COPYING.LIB file in the top-level directory. */ +#include <sys/stat.h> +#include <sys/types.h> +#include <errno.h> +#include <fcntl.h> +#include <limits.h> #include <stdbool.h> #include <stdio.h> #include <stdarg.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> #include "fuse_log.h" +#define FUSE_DYN_LOG_BASE "/run/virtiofsd" + static bool use_syslog; +static char *dyn_log_file = NULL; +static int dyn_log_fd = -1; int current_log_level = LOG_INFO; void fuse_log_enable_syslog(void) @@ -71,3 +83,135 @@ void fuse_debug(const char *fmt, ...) fuse_vlog(LOG_DEBUG, fmt, ap); va_end(ap); } + +static const char *fuse_log_level_str(int level) +{ + switch (level) { + case LOG_DEBUG: + return "debug"; + case LOG_INFO: + return "info"; + case LOG_WARNING: + return "warn"; + case LOG_ERR: + return "err"; + default: + return NULL; + } +} + +void fuse_dyn_log_init(void) +{ + struct stat stbuf; + char buf[PATH_MAX]; + const char *p; + + /* Prepare FUSE_DYN_LOG_BASE */ + if (mkdir(FUSE_DYN_LOG_BASE, 0755) == -1) { + if (errno != EEXIST) { + fuse_warning("Fail to mkdir(%s): %s\n", FUSE_DYN_LOG_BASE, + strerror(errno)); + goto err; + } + } + if (stat(FUSE_DYN_LOG_BASE, &stbuf) == -1) { + fuse_warning("Fail to stat(%s): %s\n", FUSE_DYN_LOG_BASE, + strerror(errno)); + goto err; + } + if (!S_ISDIR(stbuf.st_mode)) { + fuse_warning("%s is not a directory\n", FUSE_DYN_LOG_BASE); + goto err; + } + if (!(stbuf.st_mode & S_IRUSR) || !(stbuf.st_mode & S_IWUSR)) { + fuse_warning("Require rw permission on %s\n", FUSE_DYN_LOG_BASE); + goto err; + } + + /* Prepare log level file */ + snprintf(buf, sizeof(buf), FUSE_DYN_LOG_BASE "/log_level.%d", + getpid()); + dyn_log_file = strdup(buf); + if (dyn_log_file == NULL) { + fuse_warning("Fail to duplicate str %s: %s\n", buf, strerror(errno)); + goto err; + } + dyn_log_fd = open(dyn_log_file, O_CREAT | O_TRUNC | O_RDWR, 0644); + if (dyn_log_fd == -1) { + fuse_warning("Fail to open %s: %s\n", dyn_log_file, strerror(errno)); + goto err; + } + /* + * TODO: Due to sandboxing, unlink won't work in + * fuse_dyn_log_fin(), we have to unlink log file here and access + * it via /proc/<pid>/fd/<fd> + */ + if (unlink(dyn_log_file) == -1) + fuse_warning("Fail to unlink %s: %s\n", dyn_log_file, strerror(errno)); + + p = fuse_log_level_str(current_log_level); + if (p == NULL) { + fuse_warning("Unknown initial log level %d\n", current_log_level); + goto err; + } + if (write(dyn_log_fd, p, strlen(p)) == -1) { + fuse_warning("Fail to write (\"%s\") to %s: %s\n", p, dyn_log_file, + strerror(errno)); + } + + fuse_info("Dynamic log level adjustment is enabled, entry file %s\n", + dyn_log_file); + return; +err: + fuse_warning("Dynamic log level adjustment is disabled\n"); + return; +} + +void fuse_dyn_log_fin(void) +{ + if (dyn_log_fd != -1) { + close(dyn_log_fd); + dyn_log_fd = -1; + } + if (dyn_log_file) { + free(dyn_log_file); + dyn_log_file = NULL; + } + return; +} + +void fuse_log_handler(int sig) +{ + char buf[16] = { 0 }; + + /* FIXME: pread() causes read to hang, use lseek & read here */ + if (lseek(dyn_log_fd, 0, SEEK_SET) == -1) { + fuse_warning("Fail to seek %s: %s\n", dyn_log_file, strerror(errno)); + goto err; + } + if (read(dyn_log_fd, buf, 16) == -1 ) { + fuse_warning("Fail to read %s: %s\n", dyn_log_file, strerror(errno)); + goto err; + } + buf[15] = '\0'; + buf[strcspn(buf, "\n")] = '\0'; + + if (!strncmp(buf, "debug", 5)) { + current_log_level = LOG_DEBUG; + } else if (!strncmp(buf, "info", 4)) { + current_log_level = LOG_INFO; + } else if (!strncmp(buf, "warn", 4)) { + current_log_level = LOG_WARNING; + } else if (!strncmp(buf, "err", 3)) { + current_log_level = LOG_ERR; + } else { + fuse_warning("Unknown log level \"%s\"\n", buf); + goto err; + } + + fuse_info("Changed current log level to %s(%d)\n", buf, current_log_level); + return; +err: + fuse_warning("Reload log level failed\n"); + return; +} diff --git a/contrib/virtiofsd/fuse_log.h b/contrib/virtiofsd/fuse_log.h index c4dfc921b6d8..532a6d66e296 100644 --- a/contrib/virtiofsd/fuse_log.h +++ b/contrib/virtiofsd/fuse_log.h @@ -21,4 +21,8 @@ void fuse_warning(const char *fmt, ...) GCC_FMT_ATTR(1, 2); void fuse_info(const char *fmt, ...) GCC_FMT_ATTR(1, 2); void fuse_debug(const char *fmt, ...) GCC_FMT_ATTR(1, 2); +void fuse_dyn_log_init(void); +void fuse_dyn_log_fin(void); +void fuse_log_handler(int sig); + #endif /* FUSE_LOG_H */ diff --git a/contrib/virtiofsd/fuse_signals.c b/contrib/virtiofsd/fuse_signals.c index 9d34f6b04025..862ba1852eff 100644 --- a/contrib/virtiofsd/fuse_signals.c +++ b/contrib/virtiofsd/fuse_signals.c @@ -69,7 +69,7 @@ int fuse_set_signal_handlers(struct fuse_session *se) thus should reset to SIG_DFL in fuse_remove_signal_handlers) or if it was already set to SIG_IGN (and should be left untouched. */ - if (set_one_signal_handler(SIGHUP, exit_handler, 0) == -1 || + if (set_one_signal_handler(SIGHUP, fuse_log_handler, 0) == -1 || set_one_signal_handler(SIGINT, exit_handler, 0) == -1 || set_one_signal_handler(SIGTERM, exit_handler, 0) == -1 || set_one_signal_handler(SIGPIPE, do_nothing, 0) == -1) @@ -86,7 +86,7 @@ void fuse_remove_signal_handlers(struct fuse_session *se) else fuse_instance = NULL; - set_one_signal_handler(SIGHUP, exit_handler, 1); + set_one_signal_handler(SIGHUP, fuse_log_handler, 1); set_one_signal_handler(SIGINT, exit_handler, 1); set_one_signal_handler(SIGTERM, exit_handler, 1); set_one_signal_handler(SIGPIPE, do_nothing, 1); diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index ca11764feb88..5f32227a0712 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -2987,6 +2987,9 @@ int main(int argc, char *argv[]) get_shared(&lo, &lo.root); } + /* Initialize dynamic log level infra, after SIGHUP handler is set. */ + fuse_dyn_log_init(); + /* Must be after daemonize to get the right /proc/self/fd */ setup_proc_self_fd(&lo); @@ -2997,6 +3000,7 @@ int main(int argc, char *argv[]) /* Block until ctrl+c or fusermount -u */ ret = virtio_loop(se); + fuse_dyn_log_fin(); err_out4: fuse_session_unmount(se); err_out3: -- 2.14.4.44.g2045bb6 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-21 8:56 [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly Eryu Guan @ 2019-08-21 15:04 ` Dr. David Alan Gilbert 2019-08-22 5:07 ` piaojun 0 siblings, 1 reply; 11+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-21 15:04 UTC (permalink / raw) To: Eryu Guan; +Cc: virtio-fs * Eryu Guan (eguan@linux.alibaba.com) wrote: > Introduce a file which contains the target log level, and > current_log_level will be reloaded to the target log level on SIGHUP. > > The file is set to "/run/virtiofsd/log_level.<pid>". Due to > sandboxing in virtiofsd, this file can only be read/write via > /proc/<pid>/fd/<fd> for now. > > Example usage: > > # view current log level > cat /proc/<pid>/fd/<fd> > # change log level to debug > echo debug > /proc/<pid>/fd/<fd> > kill -HUP <pid> > > Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> This looks really complex for something that can only be used for changing log levels. Perhaps it's time we started reading stdin for some sort of simple commands? Dave > --- > contrib/virtiofsd/fuse_log.c | 144 +++++++++++++++++++++++++++++++++++++ > contrib/virtiofsd/fuse_log.h | 4 ++ > contrib/virtiofsd/fuse_signals.c | 4 +- > contrib/virtiofsd/passthrough_ll.c | 4 ++ > 4 files changed, 154 insertions(+), 2 deletions(-) > > diff --git a/contrib/virtiofsd/fuse_log.c b/contrib/virtiofsd/fuse_log.c > index d54b64099a2b..adfcbb84bfb3 100644 > --- a/contrib/virtiofsd/fuse_log.c > +++ b/contrib/virtiofsd/fuse_log.c > @@ -6,12 +6,24 @@ > * See the COPYING.LIB file in the top-level directory. > */ > > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <limits.h> > #include <stdbool.h> > #include <stdio.h> > #include <stdarg.h> > +#include <stdlib.h> > +#include <string.h> > +#include <unistd.h> > #include "fuse_log.h" > > +#define FUSE_DYN_LOG_BASE "/run/virtiofsd" > + > static bool use_syslog; > +static char *dyn_log_file = NULL; > +static int dyn_log_fd = -1; > int current_log_level = LOG_INFO; > > void fuse_log_enable_syslog(void) > @@ -71,3 +83,135 @@ void fuse_debug(const char *fmt, ...) > fuse_vlog(LOG_DEBUG, fmt, ap); > va_end(ap); > } > + > +static const char *fuse_log_level_str(int level) > +{ > + switch (level) { > + case LOG_DEBUG: > + return "debug"; > + case LOG_INFO: > + return "info"; > + case LOG_WARNING: > + return "warn"; > + case LOG_ERR: > + return "err"; > + default: > + return NULL; > + } > +} > + > +void fuse_dyn_log_init(void) > +{ > + struct stat stbuf; > + char buf[PATH_MAX]; > + const char *p; > + > + /* Prepare FUSE_DYN_LOG_BASE */ > + if (mkdir(FUSE_DYN_LOG_BASE, 0755) == -1) { > + if (errno != EEXIST) { > + fuse_warning("Fail to mkdir(%s): %s\n", FUSE_DYN_LOG_BASE, > + strerror(errno)); > + goto err; > + } > + } > + if (stat(FUSE_DYN_LOG_BASE, &stbuf) == -1) { > + fuse_warning("Fail to stat(%s): %s\n", FUSE_DYN_LOG_BASE, > + strerror(errno)); > + goto err; > + } > + if (!S_ISDIR(stbuf.st_mode)) { > + fuse_warning("%s is not a directory\n", FUSE_DYN_LOG_BASE); > + goto err; > + } > + if (!(stbuf.st_mode & S_IRUSR) || !(stbuf.st_mode & S_IWUSR)) { > + fuse_warning("Require rw permission on %s\n", FUSE_DYN_LOG_BASE); > + goto err; > + } > + > + /* Prepare log level file */ > + snprintf(buf, sizeof(buf), FUSE_DYN_LOG_BASE "/log_level.%d", > + getpid()); > + dyn_log_file = strdup(buf); > + if (dyn_log_file == NULL) { > + fuse_warning("Fail to duplicate str %s: %s\n", buf, strerror(errno)); > + goto err; > + } > + dyn_log_fd = open(dyn_log_file, O_CREAT | O_TRUNC | O_RDWR, 0644); > + if (dyn_log_fd == -1) { > + fuse_warning("Fail to open %s: %s\n", dyn_log_file, strerror(errno)); > + goto err; > + } > + /* > + * TODO: Due to sandboxing, unlink won't work in > + * fuse_dyn_log_fin(), we have to unlink log file here and access > + * it via /proc/<pid>/fd/<fd> > + */ > + if (unlink(dyn_log_file) == -1) > + fuse_warning("Fail to unlink %s: %s\n", dyn_log_file, strerror(errno)); > + > + p = fuse_log_level_str(current_log_level); > + if (p == NULL) { > + fuse_warning("Unknown initial log level %d\n", current_log_level); > + goto err; > + } > + if (write(dyn_log_fd, p, strlen(p)) == -1) { > + fuse_warning("Fail to write (\"%s\") to %s: %s\n", p, dyn_log_file, > + strerror(errno)); > + } > + > + fuse_info("Dynamic log level adjustment is enabled, entry file %s\n", > + dyn_log_file); > + return; > +err: > + fuse_warning("Dynamic log level adjustment is disabled\n"); > + return; > +} > + > +void fuse_dyn_log_fin(void) > +{ > + if (dyn_log_fd != -1) { > + close(dyn_log_fd); > + dyn_log_fd = -1; > + } > + if (dyn_log_file) { > + free(dyn_log_file); > + dyn_log_file = NULL; > + } > + return; > +} > + > +void fuse_log_handler(int sig) > +{ > + char buf[16] = { 0 }; > + > + /* FIXME: pread() causes read to hang, use lseek & read here */ > + if (lseek(dyn_log_fd, 0, SEEK_SET) == -1) { > + fuse_warning("Fail to seek %s: %s\n", dyn_log_file, strerror(errno)); > + goto err; > + } > + if (read(dyn_log_fd, buf, 16) == -1 ) { > + fuse_warning("Fail to read %s: %s\n", dyn_log_file, strerror(errno)); > + goto err; > + } > + buf[15] = '\0'; > + buf[strcspn(buf, "\n")] = '\0'; > + > + if (!strncmp(buf, "debug", 5)) { > + current_log_level = LOG_DEBUG; > + } else if (!strncmp(buf, "info", 4)) { > + current_log_level = LOG_INFO; > + } else if (!strncmp(buf, "warn", 4)) { > + current_log_level = LOG_WARNING; > + } else if (!strncmp(buf, "err", 3)) { > + current_log_level = LOG_ERR; > + } else { > + fuse_warning("Unknown log level \"%s\"\n", buf); > + goto err; > + } > + > + fuse_info("Changed current log level to %s(%d)\n", buf, current_log_level); > + return; > +err: > + fuse_warning("Reload log level failed\n"); > + return; > +} > diff --git a/contrib/virtiofsd/fuse_log.h b/contrib/virtiofsd/fuse_log.h > index c4dfc921b6d8..532a6d66e296 100644 > --- a/contrib/virtiofsd/fuse_log.h > +++ b/contrib/virtiofsd/fuse_log.h > @@ -21,4 +21,8 @@ void fuse_warning(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > void fuse_info(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > void fuse_debug(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > +void fuse_dyn_log_init(void); > +void fuse_dyn_log_fin(void); > +void fuse_log_handler(int sig); > + > #endif /* FUSE_LOG_H */ > diff --git a/contrib/virtiofsd/fuse_signals.c b/contrib/virtiofsd/fuse_signals.c > index 9d34f6b04025..862ba1852eff 100644 > --- a/contrib/virtiofsd/fuse_signals.c > +++ b/contrib/virtiofsd/fuse_signals.c > @@ -69,7 +69,7 @@ int fuse_set_signal_handlers(struct fuse_session *se) > thus should reset to SIG_DFL in fuse_remove_signal_handlers) > or if it was already set to SIG_IGN (and should be left > untouched. */ > - if (set_one_signal_handler(SIGHUP, exit_handler, 0) == -1 || > + if (set_one_signal_handler(SIGHUP, fuse_log_handler, 0) == -1 || > set_one_signal_handler(SIGINT, exit_handler, 0) == -1 || > set_one_signal_handler(SIGTERM, exit_handler, 0) == -1 || > set_one_signal_handler(SIGPIPE, do_nothing, 0) == -1) > @@ -86,7 +86,7 @@ void fuse_remove_signal_handlers(struct fuse_session *se) > else > fuse_instance = NULL; > > - set_one_signal_handler(SIGHUP, exit_handler, 1); > + set_one_signal_handler(SIGHUP, fuse_log_handler, 1); > set_one_signal_handler(SIGINT, exit_handler, 1); > set_one_signal_handler(SIGTERM, exit_handler, 1); > set_one_signal_handler(SIGPIPE, do_nothing, 1); > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index ca11764feb88..5f32227a0712 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -2987,6 +2987,9 @@ int main(int argc, char *argv[]) > get_shared(&lo, &lo.root); > } > > + /* Initialize dynamic log level infra, after SIGHUP handler is set. */ > + fuse_dyn_log_init(); > + > /* Must be after daemonize to get the right /proc/self/fd */ > setup_proc_self_fd(&lo); > > @@ -2997,6 +3000,7 @@ int main(int argc, char *argv[]) > /* Block until ctrl+c or fusermount -u */ > ret = virtio_loop(se); > > + fuse_dyn_log_fin(); > err_out4: > fuse_session_unmount(se); > err_out3: > -- > 2.14.4.44.g2045bb6 > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-21 15:04 ` Dr. David Alan Gilbert @ 2019-08-22 5:07 ` piaojun 2019-08-22 13:55 ` Stefan Hajnoczi 2019-08-22 14:16 ` Stefan Hajnoczi 0 siblings, 2 replies; 11+ messages in thread From: piaojun @ 2019-08-22 5:07 UTC (permalink / raw) To: Dr. David Alan Gilbert, Eryu Guan; +Cc: virtio-fs On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > * Eryu Guan (eguan@linux.alibaba.com) wrote: >> Introduce a file which contains the target log level, and >> current_log_level will be reloaded to the target log level on SIGHUP. >> >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to >> sandboxing in virtiofsd, this file can only be read/write via >> /proc/<pid>/fd/<fd> for now. >> >> Example usage: >> >> # view current log level >> cat /proc/<pid>/fd/<fd> >> # change log level to debug >> echo debug > /proc/<pid>/fd/<fd> >> kill -HUP <pid> >> >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > This looks really complex for something that can only be used for > changing log levels. I have the same opinion with Dave, and I wonder if we have some easier channel to tell virtiofsd to change log level. And making the channel more generic is good for the expansibility of virtiofsd. Jun > > Perhaps it's time we started reading stdin for some sort of simple > commands? > > Dave > >> --- >> contrib/virtiofsd/fuse_log.c | 144 +++++++++++++++++++++++++++++++++++++ >> contrib/virtiofsd/fuse_log.h | 4 ++ >> contrib/virtiofsd/fuse_signals.c | 4 +- >> contrib/virtiofsd/passthrough_ll.c | 4 ++ >> 4 files changed, 154 insertions(+), 2 deletions(-) >> >> diff --git a/contrib/virtiofsd/fuse_log.c b/contrib/virtiofsd/fuse_log.c >> index d54b64099a2b..adfcbb84bfb3 100644 >> --- a/contrib/virtiofsd/fuse_log.c >> +++ b/contrib/virtiofsd/fuse_log.c >> @@ -6,12 +6,24 @@ >> * See the COPYING.LIB file in the top-level directory. >> */ >> >> +#include <sys/stat.h> >> +#include <sys/types.h> >> +#include <errno.h> >> +#include <fcntl.h> >> +#include <limits.h> >> #include <stdbool.h> >> #include <stdio.h> >> #include <stdarg.h> >> +#include <stdlib.h> >> +#include <string.h> >> +#include <unistd.h> >> #include "fuse_log.h" >> >> +#define FUSE_DYN_LOG_BASE "/run/virtiofsd" >> + >> static bool use_syslog; >> +static char *dyn_log_file = NULL; >> +static int dyn_log_fd = -1; >> int current_log_level = LOG_INFO; >> >> void fuse_log_enable_syslog(void) >> @@ -71,3 +83,135 @@ void fuse_debug(const char *fmt, ...) >> fuse_vlog(LOG_DEBUG, fmt, ap); >> va_end(ap); >> } >> + >> +static const char *fuse_log_level_str(int level) >> +{ >> + switch (level) { >> + case LOG_DEBUG: >> + return "debug"; >> + case LOG_INFO: >> + return "info"; >> + case LOG_WARNING: >> + return "warn"; >> + case LOG_ERR: >> + return "err"; >> + default: >> + return NULL; >> + } >> +} >> + >> +void fuse_dyn_log_init(void) >> +{ >> + struct stat stbuf; >> + char buf[PATH_MAX]; >> + const char *p; >> + >> + /* Prepare FUSE_DYN_LOG_BASE */ >> + if (mkdir(FUSE_DYN_LOG_BASE, 0755) == -1) { >> + if (errno != EEXIST) { >> + fuse_warning("Fail to mkdir(%s): %s\n", FUSE_DYN_LOG_BASE, >> + strerror(errno)); >> + goto err; >> + } >> + } >> + if (stat(FUSE_DYN_LOG_BASE, &stbuf) == -1) { >> + fuse_warning("Fail to stat(%s): %s\n", FUSE_DYN_LOG_BASE, >> + strerror(errno)); >> + goto err; >> + } >> + if (!S_ISDIR(stbuf.st_mode)) { >> + fuse_warning("%s is not a directory\n", FUSE_DYN_LOG_BASE); >> + goto err; >> + } >> + if (!(stbuf.st_mode & S_IRUSR) || !(stbuf.st_mode & S_IWUSR)) { >> + fuse_warning("Require rw permission on %s\n", FUSE_DYN_LOG_BASE); >> + goto err; >> + } >> + >> + /* Prepare log level file */ >> + snprintf(buf, sizeof(buf), FUSE_DYN_LOG_BASE "/log_level.%d", >> + getpid()); >> + dyn_log_file = strdup(buf); >> + if (dyn_log_file == NULL) { >> + fuse_warning("Fail to duplicate str %s: %s\n", buf, strerror(errno)); >> + goto err; >> + } >> + dyn_log_fd = open(dyn_log_file, O_CREAT | O_TRUNC | O_RDWR, 0644); >> + if (dyn_log_fd == -1) { >> + fuse_warning("Fail to open %s: %s\n", dyn_log_file, strerror(errno)); >> + goto err; >> + } >> + /* >> + * TODO: Due to sandboxing, unlink won't work in >> + * fuse_dyn_log_fin(), we have to unlink log file here and access >> + * it via /proc/<pid>/fd/<fd> >> + */ >> + if (unlink(dyn_log_file) == -1) >> + fuse_warning("Fail to unlink %s: %s\n", dyn_log_file, strerror(errno)); >> + >> + p = fuse_log_level_str(current_log_level); >> + if (p == NULL) { >> + fuse_warning("Unknown initial log level %d\n", current_log_level); >> + goto err; >> + } >> + if (write(dyn_log_fd, p, strlen(p)) == -1) { >> + fuse_warning("Fail to write (\"%s\") to %s: %s\n", p, dyn_log_file, >> + strerror(errno)); >> + } >> + >> + fuse_info("Dynamic log level adjustment is enabled, entry file %s\n", >> + dyn_log_file); >> + return; >> +err: >> + fuse_warning("Dynamic log level adjustment is disabled\n"); >> + return; >> +} >> + >> +void fuse_dyn_log_fin(void) >> +{ >> + if (dyn_log_fd != -1) { >> + close(dyn_log_fd); >> + dyn_log_fd = -1; >> + } >> + if (dyn_log_file) { >> + free(dyn_log_file); >> + dyn_log_file = NULL; >> + } >> + return; >> +} >> + >> +void fuse_log_handler(int sig) >> +{ >> + char buf[16] = { 0 }; >> + >> + /* FIXME: pread() causes read to hang, use lseek & read here */ >> + if (lseek(dyn_log_fd, 0, SEEK_SET) == -1) { >> + fuse_warning("Fail to seek %s: %s\n", dyn_log_file, strerror(errno)); >> + goto err; >> + } >> + if (read(dyn_log_fd, buf, 16) == -1 ) { >> + fuse_warning("Fail to read %s: %s\n", dyn_log_file, strerror(errno)); >> + goto err; >> + } >> + buf[15] = '\0'; >> + buf[strcspn(buf, "\n")] = '\0'; >> + >> + if (!strncmp(buf, "debug", 5)) { >> + current_log_level = LOG_DEBUG; >> + } else if (!strncmp(buf, "info", 4)) { >> + current_log_level = LOG_INFO; >> + } else if (!strncmp(buf, "warn", 4)) { >> + current_log_level = LOG_WARNING; >> + } else if (!strncmp(buf, "err", 3)) { >> + current_log_level = LOG_ERR; >> + } else { >> + fuse_warning("Unknown log level \"%s\"\n", buf); >> + goto err; >> + } >> + >> + fuse_info("Changed current log level to %s(%d)\n", buf, current_log_level); >> + return; >> +err: >> + fuse_warning("Reload log level failed\n"); >> + return; >> +} >> diff --git a/contrib/virtiofsd/fuse_log.h b/contrib/virtiofsd/fuse_log.h >> index c4dfc921b6d8..532a6d66e296 100644 >> --- a/contrib/virtiofsd/fuse_log.h >> +++ b/contrib/virtiofsd/fuse_log.h >> @@ -21,4 +21,8 @@ void fuse_warning(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> void fuse_info(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> void fuse_debug(const char *fmt, ...) GCC_FMT_ATTR(1, 2); >> >> +void fuse_dyn_log_init(void); >> +void fuse_dyn_log_fin(void); >> +void fuse_log_handler(int sig); >> + >> #endif /* FUSE_LOG_H */ >> diff --git a/contrib/virtiofsd/fuse_signals.c b/contrib/virtiofsd/fuse_signals.c >> index 9d34f6b04025..862ba1852eff 100644 >> --- a/contrib/virtiofsd/fuse_signals.c >> +++ b/contrib/virtiofsd/fuse_signals.c >> @@ -69,7 +69,7 @@ int fuse_set_signal_handlers(struct fuse_session *se) >> thus should reset to SIG_DFL in fuse_remove_signal_handlers) >> or if it was already set to SIG_IGN (and should be left >> untouched. */ >> - if (set_one_signal_handler(SIGHUP, exit_handler, 0) == -1 || >> + if (set_one_signal_handler(SIGHUP, fuse_log_handler, 0) == -1 || >> set_one_signal_handler(SIGINT, exit_handler, 0) == -1 || >> set_one_signal_handler(SIGTERM, exit_handler, 0) == -1 || >> set_one_signal_handler(SIGPIPE, do_nothing, 0) == -1) >> @@ -86,7 +86,7 @@ void fuse_remove_signal_handlers(struct fuse_session *se) >> else >> fuse_instance = NULL; >> >> - set_one_signal_handler(SIGHUP, exit_handler, 1); >> + set_one_signal_handler(SIGHUP, fuse_log_handler, 1); >> set_one_signal_handler(SIGINT, exit_handler, 1); >> set_one_signal_handler(SIGTERM, exit_handler, 1); >> set_one_signal_handler(SIGPIPE, do_nothing, 1); >> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c >> index ca11764feb88..5f32227a0712 100644 >> --- a/contrib/virtiofsd/passthrough_ll.c >> +++ b/contrib/virtiofsd/passthrough_ll.c >> @@ -2987,6 +2987,9 @@ int main(int argc, char *argv[]) >> get_shared(&lo, &lo.root); >> } >> >> + /* Initialize dynamic log level infra, after SIGHUP handler is set. */ >> + fuse_dyn_log_init(); >> + >> /* Must be after daemonize to get the right /proc/self/fd */ >> setup_proc_self_fd(&lo); >> >> @@ -2997,6 +3000,7 @@ int main(int argc, char *argv[]) >> /* Block until ctrl+c or fusermount -u */ >> ret = virtio_loop(se); >> >> + fuse_dyn_log_fin(); >> err_out4: >> fuse_session_unmount(se); >> err_out3: >> -- >> 2.14.4.44.g2045bb6 >> >> _______________________________________________ >> Virtio-fs mailing list >> Virtio-fs@redhat.com >> https://www.redhat.com/mailman/listinfo/virtio-fs > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs > . > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-22 5:07 ` piaojun @ 2019-08-22 13:55 ` Stefan Hajnoczi 2019-08-22 14:16 ` Stefan Hajnoczi 1 sibling, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2019-08-22 13:55 UTC (permalink / raw) To: piaojun; +Cc: virtio-fs [-- Attachment #1: Type: text/plain, Size: 1558 bytes --] On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > >> Introduce a file which contains the target log level, and > >> current_log_level will be reloaded to the target log level on SIGHUP. > >> > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > >> sandboxing in virtiofsd, this file can only be read/write via > >> /proc/<pid>/fd/<fd> for now. > >> > >> Example usage: > >> > >> # view current log level > >> cat /proc/<pid>/fd/<fd> > >> # change log level to debug > >> echo debug > /proc/<pid>/fd/<fd> > >> kill -HUP <pid> > >> > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > This looks really complex for something that can only be used for > > changing log levels. > > I have the same opinion with Dave, and I wonder if we have some easier > channel to tell virtiofsd to change log level. And making the channel > more generic is good for the expansibility of virtiofsd. There is code for QMP (a JSON RPC-style protocol) in the QEMU codebase that can be reused. An option that is more widely used but I have little experience with is DBus. It is easy to access from most programming languages, including the command-line. Although both of these approaches are somewhat heavyweight, they scale to much more complex management commands and already have existing tooling available. I think we should choose one instead of inventing another RPC system. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-22 5:07 ` piaojun 2019-08-22 13:55 ` Stefan Hajnoczi @ 2019-08-22 14:16 ` Stefan Hajnoczi 2019-08-22 14:33 ` Marc-André Lureau 2019-08-22 15:22 ` Daniel P. Berrangé 1 sibling, 2 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2019-08-22 14:16 UTC (permalink / raw) To: piaojun; +Cc: virtio-fs, Marc-André Lureau, Daniel Berrange [-- Attachment #1: Type: text/plain, Size: 11711 bytes --] On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > >> Introduce a file which contains the target log level, and > >> current_log_level will be reloaded to the target log level on SIGHUP. > >> > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > >> sandboxing in virtiofsd, this file can only be read/write via > >> /proc/<pid>/fd/<fd> for now. > >> > >> Example usage: > >> > >> # view current log level > >> cat /proc/<pid>/fd/<fd> > >> # change log level to debug > >> echo debug > /proc/<pid>/fd/<fd> > >> kill -HUP <pid> > >> > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > This looks really complex for something that can only be used for > > changing log levels. > > I have the same opinion with Dave, and I wonder if we have some easier > channel to tell virtiofsd to change log level. And making the channel > more generic is good for the expansibility of virtiofsd. Adding Dan and Marc-André, who have dealt with DBus and QMP. They can share their experience. The long-term concern we should think about is whether vhost-user backends should have standardized management commands. They would be defined in docs/interop/vhost-user.rst and all backends implementing a device type would implement a subset of them (plus additional "vendor-specific commands" if they really don't fit into the vhost-user specification). In order to have standard commands we need to choose a standard RPC mechanism. I'm pretty sure that DBus is a good choice based on my previous discussions with Dan and Marc-André. It is easy to use from most programming languages and therefore easier to integrate than a custom management channel. Dan and Marc-André: The first commands we would like to offer in virtiofsd are: get-log-level -> NUM Return the current logging threshold level. set-log-level NUM Set the logging threshold level. If you have time to give a quick overview of how this looks in DBus that would be very helpful. The virtiofsd code is written in C and currently does not use glib/gobject/gio heavily, so I think we'd prefer a lightweight approach without a lot of gobject boilerplate, if possible. > > Jun > > > > > Perhaps it's time we started reading stdin for some sort of simple > > commands? > > > > Dave > > > >> --- > >> contrib/virtiofsd/fuse_log.c | 144 +++++++++++++++++++++++++++++++++++++ > >> contrib/virtiofsd/fuse_log.h | 4 ++ > >> contrib/virtiofsd/fuse_signals.c | 4 +- > >> contrib/virtiofsd/passthrough_ll.c | 4 ++ > >> 4 files changed, 154 insertions(+), 2 deletions(-) > >> > >> diff --git a/contrib/virtiofsd/fuse_log.c b/contrib/virtiofsd/fuse_log.c > >> index d54b64099a2b..adfcbb84bfb3 100644 > >> --- a/contrib/virtiofsd/fuse_log.c > >> +++ b/contrib/virtiofsd/fuse_log.c > >> @@ -6,12 +6,24 @@ > >> * See the COPYING.LIB file in the top-level directory. > >> */ > >> > >> +#include <sys/stat.h> > >> +#include <sys/types.h> > >> +#include <errno.h> > >> +#include <fcntl.h> > >> +#include <limits.h> > >> #include <stdbool.h> > >> #include <stdio.h> > >> #include <stdarg.h> > >> +#include <stdlib.h> > >> +#include <string.h> > >> +#include <unistd.h> > >> #include "fuse_log.h" > >> > >> +#define FUSE_DYN_LOG_BASE "/run/virtiofsd" > >> + > >> static bool use_syslog; > >> +static char *dyn_log_file = NULL; > >> +static int dyn_log_fd = -1; > >> int current_log_level = LOG_INFO; > >> > >> void fuse_log_enable_syslog(void) > >> @@ -71,3 +83,135 @@ void fuse_debug(const char *fmt, ...) > >> fuse_vlog(LOG_DEBUG, fmt, ap); > >> va_end(ap); > >> } > >> + > >> +static const char *fuse_log_level_str(int level) > >> +{ > >> + switch (level) { > >> + case LOG_DEBUG: > >> + return "debug"; > >> + case LOG_INFO: > >> + return "info"; > >> + case LOG_WARNING: > >> + return "warn"; > >> + case LOG_ERR: > >> + return "err"; > >> + default: > >> + return NULL; > >> + } > >> +} > >> + > >> +void fuse_dyn_log_init(void) > >> +{ > >> + struct stat stbuf; > >> + char buf[PATH_MAX]; > >> + const char *p; > >> + > >> + /* Prepare FUSE_DYN_LOG_BASE */ > >> + if (mkdir(FUSE_DYN_LOG_BASE, 0755) == -1) { > >> + if (errno != EEXIST) { > >> + fuse_warning("Fail to mkdir(%s): %s\n", FUSE_DYN_LOG_BASE, > >> + strerror(errno)); > >> + goto err; > >> + } > >> + } > >> + if (stat(FUSE_DYN_LOG_BASE, &stbuf) == -1) { > >> + fuse_warning("Fail to stat(%s): %s\n", FUSE_DYN_LOG_BASE, > >> + strerror(errno)); > >> + goto err; > >> + } > >> + if (!S_ISDIR(stbuf.st_mode)) { > >> + fuse_warning("%s is not a directory\n", FUSE_DYN_LOG_BASE); > >> + goto err; > >> + } > >> + if (!(stbuf.st_mode & S_IRUSR) || !(stbuf.st_mode & S_IWUSR)) { > >> + fuse_warning("Require rw permission on %s\n", FUSE_DYN_LOG_BASE); > >> + goto err; > >> + } > >> + > >> + /* Prepare log level file */ > >> + snprintf(buf, sizeof(buf), FUSE_DYN_LOG_BASE "/log_level.%d", > >> + getpid()); > >> + dyn_log_file = strdup(buf); > >> + if (dyn_log_file == NULL) { > >> + fuse_warning("Fail to duplicate str %s: %s\n", buf, strerror(errno)); > >> + goto err; > >> + } > >> + dyn_log_fd = open(dyn_log_file, O_CREAT | O_TRUNC | O_RDWR, 0644); > >> + if (dyn_log_fd == -1) { > >> + fuse_warning("Fail to open %s: %s\n", dyn_log_file, strerror(errno)); > >> + goto err; > >> + } > >> + /* > >> + * TODO: Due to sandboxing, unlink won't work in > >> + * fuse_dyn_log_fin(), we have to unlink log file here and access > >> + * it via /proc/<pid>/fd/<fd> > >> + */ > >> + if (unlink(dyn_log_file) == -1) > >> + fuse_warning("Fail to unlink %s: %s\n", dyn_log_file, strerror(errno)); > >> + > >> + p = fuse_log_level_str(current_log_level); > >> + if (p == NULL) { > >> + fuse_warning("Unknown initial log level %d\n", current_log_level); > >> + goto err; > >> + } > >> + if (write(dyn_log_fd, p, strlen(p)) == -1) { > >> + fuse_warning("Fail to write (\"%s\") to %s: %s\n", p, dyn_log_file, > >> + strerror(errno)); > >> + } > >> + > >> + fuse_info("Dynamic log level adjustment is enabled, entry file %s\n", > >> + dyn_log_file); > >> + return; > >> +err: > >> + fuse_warning("Dynamic log level adjustment is disabled\n"); > >> + return; > >> +} > >> + > >> +void fuse_dyn_log_fin(void) > >> +{ > >> + if (dyn_log_fd != -1) { > >> + close(dyn_log_fd); > >> + dyn_log_fd = -1; > >> + } > >> + if (dyn_log_file) { > >> + free(dyn_log_file); > >> + dyn_log_file = NULL; > >> + } > >> + return; > >> +} > >> + > >> +void fuse_log_handler(int sig) > >> +{ > >> + char buf[16] = { 0 }; > >> + > >> + /* FIXME: pread() causes read to hang, use lseek & read here */ > >> + if (lseek(dyn_log_fd, 0, SEEK_SET) == -1) { > >> + fuse_warning("Fail to seek %s: %s\n", dyn_log_file, strerror(errno)); > >> + goto err; > >> + } > >> + if (read(dyn_log_fd, buf, 16) == -1 ) { > >> + fuse_warning("Fail to read %s: %s\n", dyn_log_file, strerror(errno)); > >> + goto err; > >> + } > >> + buf[15] = '\0'; > >> + buf[strcspn(buf, "\n")] = '\0'; > >> + > >> + if (!strncmp(buf, "debug", 5)) { > >> + current_log_level = LOG_DEBUG; > >> + } else if (!strncmp(buf, "info", 4)) { > >> + current_log_level = LOG_INFO; > >> + } else if (!strncmp(buf, "warn", 4)) { > >> + current_log_level = LOG_WARNING; > >> + } else if (!strncmp(buf, "err", 3)) { > >> + current_log_level = LOG_ERR; > >> + } else { > >> + fuse_warning("Unknown log level \"%s\"\n", buf); > >> + goto err; > >> + } > >> + > >> + fuse_info("Changed current log level to %s(%d)\n", buf, current_log_level); > >> + return; > >> +err: > >> + fuse_warning("Reload log level failed\n"); > >> + return; > >> +} > >> diff --git a/contrib/virtiofsd/fuse_log.h b/contrib/virtiofsd/fuse_log.h > >> index c4dfc921b6d8..532a6d66e296 100644 > >> --- a/contrib/virtiofsd/fuse_log.h > >> +++ b/contrib/virtiofsd/fuse_log.h > >> @@ -21,4 +21,8 @@ void fuse_warning(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > >> void fuse_info(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > >> void fuse_debug(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > >> > >> +void fuse_dyn_log_init(void); > >> +void fuse_dyn_log_fin(void); > >> +void fuse_log_handler(int sig); > >> + > >> #endif /* FUSE_LOG_H */ > >> diff --git a/contrib/virtiofsd/fuse_signals.c b/contrib/virtiofsd/fuse_signals.c > >> index 9d34f6b04025..862ba1852eff 100644 > >> --- a/contrib/virtiofsd/fuse_signals.c > >> +++ b/contrib/virtiofsd/fuse_signals.c > >> @@ -69,7 +69,7 @@ int fuse_set_signal_handlers(struct fuse_session *se) > >> thus should reset to SIG_DFL in fuse_remove_signal_handlers) > >> or if it was already set to SIG_IGN (and should be left > >> untouched. */ > >> - if (set_one_signal_handler(SIGHUP, exit_handler, 0) == -1 || > >> + if (set_one_signal_handler(SIGHUP, fuse_log_handler, 0) == -1 || > >> set_one_signal_handler(SIGINT, exit_handler, 0) == -1 || > >> set_one_signal_handler(SIGTERM, exit_handler, 0) == -1 || > >> set_one_signal_handler(SIGPIPE, do_nothing, 0) == -1) > >> @@ -86,7 +86,7 @@ void fuse_remove_signal_handlers(struct fuse_session *se) > >> else > >> fuse_instance = NULL; > >> > >> - set_one_signal_handler(SIGHUP, exit_handler, 1); > >> + set_one_signal_handler(SIGHUP, fuse_log_handler, 1); > >> set_one_signal_handler(SIGINT, exit_handler, 1); > >> set_one_signal_handler(SIGTERM, exit_handler, 1); > >> set_one_signal_handler(SIGPIPE, do_nothing, 1); > >> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > >> index ca11764feb88..5f32227a0712 100644 > >> --- a/contrib/virtiofsd/passthrough_ll.c > >> +++ b/contrib/virtiofsd/passthrough_ll.c > >> @@ -2987,6 +2987,9 @@ int main(int argc, char *argv[]) > >> get_shared(&lo, &lo.root); > >> } > >> > >> + /* Initialize dynamic log level infra, after SIGHUP handler is set. */ > >> + fuse_dyn_log_init(); > >> + > >> /* Must be after daemonize to get the right /proc/self/fd */ > >> setup_proc_self_fd(&lo); > >> > >> @@ -2997,6 +3000,7 @@ int main(int argc, char *argv[]) > >> /* Block until ctrl+c or fusermount -u */ > >> ret = virtio_loop(se); > >> > >> + fuse_dyn_log_fin(); > >> err_out4: > >> fuse_session_unmount(se); > >> err_out3: > >> -- > >> 2.14.4.44.g2045bb6 > >> > >> _______________________________________________ > >> Virtio-fs mailing list > >> Virtio-fs@redhat.com > >> https://www.redhat.com/mailman/listinfo/virtio-fs > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > _______________________________________________ > > Virtio-fs mailing list > > Virtio-fs@redhat.com > > https://www.redhat.com/mailman/listinfo/virtio-fs > > . > > > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-22 14:16 ` Stefan Hajnoczi @ 2019-08-22 14:33 ` Marc-André Lureau 2019-08-22 15:35 ` Daniel P. Berrangé 2019-08-22 15:22 ` Daniel P. Berrangé 1 sibling, 1 reply; 11+ messages in thread From: Marc-André Lureau @ 2019-08-22 14:33 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, Daniel Berrange Hi On Thu, Aug 22, 2019 at 6:16 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > > >> Introduce a file which contains the target log level, and > > >> current_log_level will be reloaded to the target log level on SIGHUP. > > >> > > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > > >> sandboxing in virtiofsd, this file can only be read/write via > > >> /proc/<pid>/fd/<fd> for now. > > >> > > >> Example usage: > > >> > > >> # view current log level > > >> cat /proc/<pid>/fd/<fd> > > >> # change log level to debug > > >> echo debug > /proc/<pid>/fd/<fd> > > >> kill -HUP <pid> > > >> > > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > > > This looks really complex for something that can only be used for > > > changing log levels. > > > > I have the same opinion with Dave, and I wonder if we have some easier > > channel to tell virtiofsd to change log level. And making the channel > > more generic is good for the expansibility of virtiofsd. > > Adding Dan and Marc-André, who have dealt with DBus and QMP. They can > share their experience. > > The long-term concern we should think about is whether vhost-user > backends should have standardized management commands. They would be > defined in docs/interop/vhost-user.rst and all backends implementing a > device type would implement a subset of them (plus additional > "vendor-specific commands" if they really don't fit into the vhost-user > specification). > > In order to have standard commands we need to choose a standard RPC > mechanism. I'm pretty sure that DBus is a good choice based on my > previous discussions with Dan and Marc-André. It is easy to use from > most programming languages and therefore easier to integrate than a > custom management channel. > > Dan and Marc-André: The first commands we would like to offer in > virtiofsd are: > > get-log-level -> NUM > Return the current logging threshold level. > > set-log-level NUM > Set the logging threshold level. > > If you have time to give a quick overview of how this looks in DBus that > would be very helpful. The virtiofsd code is written in C and currently Well that depends how generic we want the interface to be :) https://dbus.freedesktop.org/doc/dbus-api-design.html service-name: none, or org.qemu.VhostUser1 or even org.qemu.VirtioFs1 ? iface: org.qemu.VhostUser1, org.qemu.VirtioFs1 ? path: /org/qemu/VhostUser1 property: "LogLevel", readwrite, string. If you follow systemd/syslogd: "this accepts a numerical log level or the well-known syslog(3) symbolic names (lowercase): emerg, alert, crit, err, warning, notice, info, debug." Systemd also has "LogTarget" and a bunch of other options/properties that are settable from command line and at runtime with dbus (see man page). > does not use glib/gobject/gio heavily, so I think we'd prefer a > lightweight approach without a lot of gobject boilerplate, if possible. If you can afford GIO, I would highly recommend it. Even if you have to run it in a seperate thread, that may be easier to maintain. Otherwise, sdbus or libdbus. > > > > > Jun > > > > > > > > Perhaps it's time we started reading stdin for some sort of simple > > > commands? > > > > > > Dave > > > > > >> --- > > >> contrib/virtiofsd/fuse_log.c | 144 +++++++++++++++++++++++++++++++++++++ > > >> contrib/virtiofsd/fuse_log.h | 4 ++ > > >> contrib/virtiofsd/fuse_signals.c | 4 +- > > >> contrib/virtiofsd/passthrough_ll.c | 4 ++ > > >> 4 files changed, 154 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/contrib/virtiofsd/fuse_log.c b/contrib/virtiofsd/fuse_log.c > > >> index d54b64099a2b..adfcbb84bfb3 100644 > > >> --- a/contrib/virtiofsd/fuse_log.c > > >> +++ b/contrib/virtiofsd/fuse_log.c > > >> @@ -6,12 +6,24 @@ > > >> * See the COPYING.LIB file in the top-level directory. > > >> */ > > >> > > >> +#include <sys/stat.h> > > >> +#include <sys/types.h> > > >> +#include <errno.h> > > >> +#include <fcntl.h> > > >> +#include <limits.h> > > >> #include <stdbool.h> > > >> #include <stdio.h> > > >> #include <stdarg.h> > > >> +#include <stdlib.h> > > >> +#include <string.h> > > >> +#include <unistd.h> > > >> #include "fuse_log.h" > > >> > > >> +#define FUSE_DYN_LOG_BASE "/run/virtiofsd" > > >> + > > >> static bool use_syslog; > > >> +static char *dyn_log_file = NULL; > > >> +static int dyn_log_fd = -1; > > >> int current_log_level = LOG_INFO; > > >> > > >> void fuse_log_enable_syslog(void) > > >> @@ -71,3 +83,135 @@ void fuse_debug(const char *fmt, ...) > > >> fuse_vlog(LOG_DEBUG, fmt, ap); > > >> va_end(ap); > > >> } > > >> + > > >> +static const char *fuse_log_level_str(int level) > > >> +{ > > >> + switch (level) { > > >> + case LOG_DEBUG: > > >> + return "debug"; > > >> + case LOG_INFO: > > >> + return "info"; > > >> + case LOG_WARNING: > > >> + return "warn"; > > >> + case LOG_ERR: > > >> + return "err"; > > >> + default: > > >> + return NULL; > > >> + } > > >> +} > > >> + > > >> +void fuse_dyn_log_init(void) > > >> +{ > > >> + struct stat stbuf; > > >> + char buf[PATH_MAX]; > > >> + const char *p; > > >> + > > >> + /* Prepare FUSE_DYN_LOG_BASE */ > > >> + if (mkdir(FUSE_DYN_LOG_BASE, 0755) == -1) { > > >> + if (errno != EEXIST) { > > >> + fuse_warning("Fail to mkdir(%s): %s\n", FUSE_DYN_LOG_BASE, > > >> + strerror(errno)); > > >> + goto err; > > >> + } > > >> + } > > >> + if (stat(FUSE_DYN_LOG_BASE, &stbuf) == -1) { > > >> + fuse_warning("Fail to stat(%s): %s\n", FUSE_DYN_LOG_BASE, > > >> + strerror(errno)); > > >> + goto err; > > >> + } > > >> + if (!S_ISDIR(stbuf.st_mode)) { > > >> + fuse_warning("%s is not a directory\n", FUSE_DYN_LOG_BASE); > > >> + goto err; > > >> + } > > >> + if (!(stbuf.st_mode & S_IRUSR) || !(stbuf.st_mode & S_IWUSR)) { > > >> + fuse_warning("Require rw permission on %s\n", FUSE_DYN_LOG_BASE); > > >> + goto err; > > >> + } > > >> + > > >> + /* Prepare log level file */ > > >> + snprintf(buf, sizeof(buf), FUSE_DYN_LOG_BASE "/log_level.%d", > > >> + getpid()); > > >> + dyn_log_file = strdup(buf); > > >> + if (dyn_log_file == NULL) { > > >> + fuse_warning("Fail to duplicate str %s: %s\n", buf, strerror(errno)); > > >> + goto err; > > >> + } > > >> + dyn_log_fd = open(dyn_log_file, O_CREAT | O_TRUNC | O_RDWR, 0644); > > >> + if (dyn_log_fd == -1) { > > >> + fuse_warning("Fail to open %s: %s\n", dyn_log_file, strerror(errno)); > > >> + goto err; > > >> + } > > >> + /* > > >> + * TODO: Due to sandboxing, unlink won't work in > > >> + * fuse_dyn_log_fin(), we have to unlink log file here and access > > >> + * it via /proc/<pid>/fd/<fd> > > >> + */ > > >> + if (unlink(dyn_log_file) == -1) > > >> + fuse_warning("Fail to unlink %s: %s\n", dyn_log_file, strerror(errno)); > > >> + > > >> + p = fuse_log_level_str(current_log_level); > > >> + if (p == NULL) { > > >> + fuse_warning("Unknown initial log level %d\n", current_log_level); > > >> + goto err; > > >> + } > > >> + if (write(dyn_log_fd, p, strlen(p)) == -1) { > > >> + fuse_warning("Fail to write (\"%s\") to %s: %s\n", p, dyn_log_file, > > >> + strerror(errno)); > > >> + } > > >> + > > >> + fuse_info("Dynamic log level adjustment is enabled, entry file %s\n", > > >> + dyn_log_file); > > >> + return; > > >> +err: > > >> + fuse_warning("Dynamic log level adjustment is disabled\n"); > > >> + return; > > >> +} > > >> + > > >> +void fuse_dyn_log_fin(void) > > >> +{ > > >> + if (dyn_log_fd != -1) { > > >> + close(dyn_log_fd); > > >> + dyn_log_fd = -1; > > >> + } > > >> + if (dyn_log_file) { > > >> + free(dyn_log_file); > > >> + dyn_log_file = NULL; > > >> + } > > >> + return; > > >> +} > > >> + > > >> +void fuse_log_handler(int sig) > > >> +{ > > >> + char buf[16] = { 0 }; > > >> + > > >> + /* FIXME: pread() causes read to hang, use lseek & read here */ > > >> + if (lseek(dyn_log_fd, 0, SEEK_SET) == -1) { > > >> + fuse_warning("Fail to seek %s: %s\n", dyn_log_file, strerror(errno)); > > >> + goto err; > > >> + } > > >> + if (read(dyn_log_fd, buf, 16) == -1 ) { > > >> + fuse_warning("Fail to read %s: %s\n", dyn_log_file, strerror(errno)); > > >> + goto err; > > >> + } > > >> + buf[15] = '\0'; > > >> + buf[strcspn(buf, "\n")] = '\0'; > > >> + > > >> + if (!strncmp(buf, "debug", 5)) { > > >> + current_log_level = LOG_DEBUG; > > >> + } else if (!strncmp(buf, "info", 4)) { > > >> + current_log_level = LOG_INFO; > > >> + } else if (!strncmp(buf, "warn", 4)) { > > >> + current_log_level = LOG_WARNING; > > >> + } else if (!strncmp(buf, "err", 3)) { > > >> + current_log_level = LOG_ERR; > > >> + } else { > > >> + fuse_warning("Unknown log level \"%s\"\n", buf); > > >> + goto err; > > >> + } > > >> + > > >> + fuse_info("Changed current log level to %s(%d)\n", buf, current_log_level); > > >> + return; > > >> +err: > > >> + fuse_warning("Reload log level failed\n"); > > >> + return; > > >> +} > > >> diff --git a/contrib/virtiofsd/fuse_log.h b/contrib/virtiofsd/fuse_log.h > > >> index c4dfc921b6d8..532a6d66e296 100644 > > >> --- a/contrib/virtiofsd/fuse_log.h > > >> +++ b/contrib/virtiofsd/fuse_log.h > > >> @@ -21,4 +21,8 @@ void fuse_warning(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > >> void fuse_info(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > >> void fuse_debug(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > >> > > >> +void fuse_dyn_log_init(void); > > >> +void fuse_dyn_log_fin(void); > > >> +void fuse_log_handler(int sig); > > >> + > > >> #endif /* FUSE_LOG_H */ > > >> diff --git a/contrib/virtiofsd/fuse_signals.c b/contrib/virtiofsd/fuse_signals.c > > >> index 9d34f6b04025..862ba1852eff 100644 > > >> --- a/contrib/virtiofsd/fuse_signals.c > > >> +++ b/contrib/virtiofsd/fuse_signals.c > > >> @@ -69,7 +69,7 @@ int fuse_set_signal_handlers(struct fuse_session *se) > > >> thus should reset to SIG_DFL in fuse_remove_signal_handlers) > > >> or if it was already set to SIG_IGN (and should be left > > >> untouched. */ > > >> - if (set_one_signal_handler(SIGHUP, exit_handler, 0) == -1 || > > >> + if (set_one_signal_handler(SIGHUP, fuse_log_handler, 0) == -1 || > > >> set_one_signal_handler(SIGINT, exit_handler, 0) == -1 || > > >> set_one_signal_handler(SIGTERM, exit_handler, 0) == -1 || > > >> set_one_signal_handler(SIGPIPE, do_nothing, 0) == -1) > > >> @@ -86,7 +86,7 @@ void fuse_remove_signal_handlers(struct fuse_session *se) > > >> else > > >> fuse_instance = NULL; > > >> > > >> - set_one_signal_handler(SIGHUP, exit_handler, 1); > > >> + set_one_signal_handler(SIGHUP, fuse_log_handler, 1); > > >> set_one_signal_handler(SIGINT, exit_handler, 1); > > >> set_one_signal_handler(SIGTERM, exit_handler, 1); > > >> set_one_signal_handler(SIGPIPE, do_nothing, 1); > > >> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > > >> index ca11764feb88..5f32227a0712 100644 > > >> --- a/contrib/virtiofsd/passthrough_ll.c > > >> +++ b/contrib/virtiofsd/passthrough_ll.c > > >> @@ -2987,6 +2987,9 @@ int main(int argc, char *argv[]) > > >> get_shared(&lo, &lo.root); > > >> } > > >> > > >> + /* Initialize dynamic log level infra, after SIGHUP handler is set. */ > > >> + fuse_dyn_log_init(); > > >> + > > >> /* Must be after daemonize to get the right /proc/self/fd */ > > >> setup_proc_self_fd(&lo); > > >> > > >> @@ -2997,6 +3000,7 @@ int main(int argc, char *argv[]) > > >> /* Block until ctrl+c or fusermount -u */ > > >> ret = virtio_loop(se); > > >> > > >> + fuse_dyn_log_fin(); > > >> err_out4: > > >> fuse_session_unmount(se); > > >> err_out3: > > >> -- > > >> 2.14.4.44.g2045bb6 > > >> > > >> _______________________________________________ > > >> Virtio-fs mailing list > > >> Virtio-fs@redhat.com > > >> https://www.redhat.com/mailman/listinfo/virtio-fs > > > -- > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > > > _______________________________________________ > > > Virtio-fs mailing list > > > Virtio-fs@redhat.com > > > https://www.redhat.com/mailman/listinfo/virtio-fs > > > . > > > > > > > _______________________________________________ > > Virtio-fs mailing list > > Virtio-fs@redhat.com > > https://www.redhat.com/mailman/listinfo/virtio-fs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-22 14:33 ` Marc-André Lureau @ 2019-08-22 15:35 ` Daniel P. Berrangé 2019-08-23 10:51 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Daniel P. Berrangé @ 2019-08-22 15:35 UTC (permalink / raw) To: Marc-André Lureau; +Cc: virtio-fs On Thu, Aug 22, 2019 at 06:33:05PM +0400, Marc-André Lureau wrote: > Hi > > On Thu, Aug 22, 2019 at 6:16 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > > > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > > > >> Introduce a file which contains the target log level, and > > > >> current_log_level will be reloaded to the target log level on SIGHUP. > > > >> > > > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > > > >> sandboxing in virtiofsd, this file can only be read/write via > > > >> /proc/<pid>/fd/<fd> for now. > > > >> > > > >> Example usage: > > > >> > > > >> # view current log level > > > >> cat /proc/<pid>/fd/<fd> > > > >> # change log level to debug > > > >> echo debug > /proc/<pid>/fd/<fd> > > > >> kill -HUP <pid> > > > >> > > > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > > > > > This looks really complex for something that can only be used for > > > > changing log levels. > > > > > > I have the same opinion with Dave, and I wonder if we have some easier > > > channel to tell virtiofsd to change log level. And making the channel > > > more generic is good for the expansibility of virtiofsd. > > > > Adding Dan and Marc-André, who have dealt with DBus and QMP. They can > > share their experience. > > > > The long-term concern we should think about is whether vhost-user > > backends should have standardized management commands. They would be > > defined in docs/interop/vhost-user.rst and all backends implementing a > > device type would implement a subset of them (plus additional > > "vendor-specific commands" if they really don't fit into the vhost-user > > specification). > > > > In order to have standard commands we need to choose a standard RPC > > mechanism. I'm pretty sure that DBus is a good choice based on my > > previous discussions with Dan and Marc-André. It is easy to use from > > most programming languages and therefore easier to integrate than a > > custom management channel. > > > > Dan and Marc-André: The first commands we would like to offer in > > virtiofsd are: > > > > get-log-level -> NUM > > Return the current logging threshold level. > > > > set-log-level NUM > > Set the logging threshold level. > > > > If you have time to give a quick overview of how this looks in DBus that > > would be very helpful. The virtiofsd code is written in C and currently > > Well that depends how generic we want the interface to be :) > > https://dbus.freedesktop.org/doc/dbus-api-design.html > > service-name: none, or org.qemu.VhostUser1 or even org.qemu.VirtioFs1 ? > iface: org.qemu.VhostUser1, org.qemu.VirtioFs1 ? > path: /org/qemu/VhostUser1 > property: "LogLevel", readwrite, string. If you follow systemd/syslogd: > "this accepts a numerical log level or the well-known syslog(3) > symbolic names (lowercase): emerg, alert, crit, err, warning, notice, > info, debug." > > Systemd also has "LogTarget" and a bunch of other options/properties > that are settable from command line and at runtime with dbus (see man > page). > > > > does not use glib/gobject/gio heavily, so I think we'd prefer a > > lightweight approach without a lot of gobject boilerplate, if possible. > > If you can afford GIO, I would highly recommend it. Even if you have > to run it in a seperate thread, that may be easier to maintain. > > Otherwise, sdbus or libdbus. sdbus is fairly nice if you absolutely must avoid GIO, but per my other msg I'd strongly recommend just using GIO for C apps. I would strongly recommend staying well away from libdbus. It is a very low level API and has some very unfortunate design choices which especially impact threaded usage. Most widely used DBus impls have dropped / avoided libdbus these days and just impl the wire protocol themselves. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-22 15:35 ` Daniel P. Berrangé @ 2019-08-23 10:51 ` Stefan Hajnoczi 2019-08-26 9:18 ` Eryu Guan 0 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2019-08-23 10:51 UTC (permalink / raw) To: Eryu Guan; +Cc: Marc-André Lureau, virtio-fs, Daniel Berrange [-- Attachment #1: Type: text/plain, Size: 4939 bytes --] On Thu, Aug 22, 2019 at 04:35:03PM +0100, Daniel P. Berrangé wrote: > On Thu, Aug 22, 2019 at 06:33:05PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Thu, Aug 22, 2019 at 6:16 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > > On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > > > > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > > > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > > > > >> Introduce a file which contains the target log level, and > > > > >> current_log_level will be reloaded to the target log level on SIGHUP. > > > > >> > > > > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > > > > >> sandboxing in virtiofsd, this file can only be read/write via > > > > >> /proc/<pid>/fd/<fd> for now. > > > > >> > > > > >> Example usage: > > > > >> > > > > >> # view current log level > > > > >> cat /proc/<pid>/fd/<fd> > > > > >> # change log level to debug > > > > >> echo debug > /proc/<pid>/fd/<fd> > > > > >> kill -HUP <pid> > > > > >> > > > > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > > > > > > > This looks really complex for something that can only be used for > > > > > changing log levels. > > > > > > > > I have the same opinion with Dave, and I wonder if we have some easier > > > > channel to tell virtiofsd to change log level. And making the channel > > > > more generic is good for the expansibility of virtiofsd. > > > > > > Adding Dan and Marc-André, who have dealt with DBus and QMP. They can > > > share their experience. > > > > > > The long-term concern we should think about is whether vhost-user > > > backends should have standardized management commands. They would be > > > defined in docs/interop/vhost-user.rst and all backends implementing a > > > device type would implement a subset of them (plus additional > > > "vendor-specific commands" if they really don't fit into the vhost-user > > > specification). > > > > > > In order to have standard commands we need to choose a standard RPC > > > mechanism. I'm pretty sure that DBus is a good choice based on my > > > previous discussions with Dan and Marc-André. It is easy to use from > > > most programming languages and therefore easier to integrate than a > > > custom management channel. > > > > > > Dan and Marc-André: The first commands we would like to offer in > > > virtiofsd are: > > > > > > get-log-level -> NUM > > > Return the current logging threshold level. > > > > > > set-log-level NUM > > > Set the logging threshold level. > > > > > > If you have time to give a quick overview of how this looks in DBus that > > > would be very helpful. The virtiofsd code is written in C and currently > > > > Well that depends how generic we want the interface to be :) > > > > https://dbus.freedesktop.org/doc/dbus-api-design.html > > > > service-name: none, or org.qemu.VhostUser1 or even org.qemu.VirtioFs1 ? > > iface: org.qemu.VhostUser1, org.qemu.VirtioFs1 ? > > path: /org/qemu/VhostUser1 > > property: "LogLevel", readwrite, string. If you follow systemd/syslogd: > > "this accepts a numerical log level or the well-known syslog(3) > > symbolic names (lowercase): emerg, alert, crit, err, warning, notice, > > info, debug." > > > > Systemd also has "LogTarget" and a bunch of other options/properties > > that are settable from command line and at runtime with dbus (see man > > page). > > > > > > > does not use glib/gobject/gio heavily, so I think we'd prefer a > > > lightweight approach without a lot of gobject boilerplate, if possible. > > > > If you can afford GIO, I would highly recommend it. Even if you have > > to run it in a seperate thread, that may be easier to maintain. > > > > Otherwise, sdbus or libdbus. > > sdbus is fairly nice if you absolutely must avoid GIO, but per my > other msg I'd strongly recommend just using GIO for C apps. > > I would strongly recommend staying well away from libdbus. It is > a very low level API and has some very unfortunate design choices > which especially impact threaded usage. Most widely used DBus impls > have dropped / avoided libdbus these days and just impl the wire > protocol themselves. Thanks for the information. I have just seen the dbus-vmstate patch on qemu-devel and I guess vhost-user will eventually use that for live migration state. So we'll have DBus code in virtiofsd anyway. I think this makes the choice to use DBus an easy one. Eryu Guan: Sorry that your patch series was side-tracked by a discussion about adding a management interface to virtiofsd. But I think we should start with DBus (based on what Daniel and Marc-André have suggested) so that it's easy to add more management commands in the future. Do you want to try this out? Otherwise I can try to rework your patch using DBus so we can see what it would look like. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-23 10:51 ` Stefan Hajnoczi @ 2019-08-26 9:18 ` Eryu Guan 2019-08-27 14:45 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Eryu Guan @ 2019-08-26 9:18 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Marc-André Lureau, virtio-fs, Daniel Berrange On Fri, Aug 23, 2019 at 11:51:43AM +0100, Stefan Hajnoczi wrote: > On Thu, Aug 22, 2019 at 04:35:03PM +0100, Daniel P. Berrangé wrote: > > On Thu, Aug 22, 2019 at 06:33:05PM +0400, Marc-André Lureau wrote: > > > Hi > > > > > > On Thu, Aug 22, 2019 at 6:16 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > > > > On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > > > > > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > > > > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > > > > > >> Introduce a file which contains the target log level, and > > > > > >> current_log_level will be reloaded to the target log level on SIGHUP. > > > > > >> > > > > > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > > > > > >> sandboxing in virtiofsd, this file can only be read/write via > > > > > >> /proc/<pid>/fd/<fd> for now. > > > > > >> > > > > > >> Example usage: > > > > > >> > > > > > >> # view current log level > > > > > >> cat /proc/<pid>/fd/<fd> > > > > > >> # change log level to debug > > > > > >> echo debug > /proc/<pid>/fd/<fd> > > > > > >> kill -HUP <pid> > > > > > >> > > > > > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > > > > > > > > > This looks really complex for something that can only be used for > > > > > > changing log levels. > > > > > > > > > > I have the same opinion with Dave, and I wonder if we have some easier > > > > > channel to tell virtiofsd to change log level. And making the channel > > > > > more generic is good for the expansibility of virtiofsd. > > > > > > > > Adding Dan and Marc-André, who have dealt with DBus and QMP. They can > > > > share their experience. > > > > > > > > The long-term concern we should think about is whether vhost-user > > > > backends should have standardized management commands. They would be > > > > defined in docs/interop/vhost-user.rst and all backends implementing a > > > > device type would implement a subset of them (plus additional > > > > "vendor-specific commands" if they really don't fit into the vhost-user > > > > specification). > > > > > > > > In order to have standard commands we need to choose a standard RPC > > > > mechanism. I'm pretty sure that DBus is a good choice based on my > > > > previous discussions with Dan and Marc-André. It is easy to use from > > > > most programming languages and therefore easier to integrate than a > > > > custom management channel. > > > > > > > > Dan and Marc-André: The first commands we would like to offer in > > > > virtiofsd are: > > > > > > > > get-log-level -> NUM > > > > Return the current logging threshold level. > > > > > > > > set-log-level NUM > > > > Set the logging threshold level. > > > > > > > > If you have time to give a quick overview of how this looks in DBus that > > > > would be very helpful. The virtiofsd code is written in C and currently > > > > > > Well that depends how generic we want the interface to be :) > > > > > > https://dbus.freedesktop.org/doc/dbus-api-design.html > > > > > > service-name: none, or org.qemu.VhostUser1 or even org.qemu.VirtioFs1 ? > > > iface: org.qemu.VhostUser1, org.qemu.VirtioFs1 ? > > > path: /org/qemu/VhostUser1 > > > property: "LogLevel", readwrite, string. If you follow systemd/syslogd: > > > "this accepts a numerical log level or the well-known syslog(3) > > > symbolic names (lowercase): emerg, alert, crit, err, warning, notice, > > > info, debug." > > > > > > Systemd also has "LogTarget" and a bunch of other options/properties > > > that are settable from command line and at runtime with dbus (see man > > > page). > > > > > > > > > > does not use glib/gobject/gio heavily, so I think we'd prefer a > > > > lightweight approach without a lot of gobject boilerplate, if possible. > > > > > > If you can afford GIO, I would highly recommend it. Even if you have > > > to run it in a seperate thread, that may be easier to maintain. > > > > > > Otherwise, sdbus or libdbus. > > > > sdbus is fairly nice if you absolutely must avoid GIO, but per my > > other msg I'd strongly recommend just using GIO for C apps. > > > > I would strongly recommend staying well away from libdbus. It is > > a very low level API and has some very unfortunate design choices > > which especially impact threaded usage. Most widely used DBus impls > > have dropped / avoided libdbus these days and just impl the wire > > protocol themselves. > > Thanks for the information. > > I have just seen the dbus-vmstate patch on qemu-devel and I guess > vhost-user will eventually use that for live migration state. So we'll > have DBus code in virtiofsd anyway. > > I think this makes the choice to use DBus an easy one. > > Eryu Guan: Sorry that your patch series was side-tracked by a discussion > about adding a management interface to virtiofsd. But I think we should Glad to see the community reaches an agreement on this :) > start with DBus (based on what Daniel and Marc-André have suggested) so > that it's easy to add more management commands in the future. Do you > want to try this out? Otherwise I can try to rework your patch using > DBus so we can see what it would look like. I have zero experience on DBus, and I'm afraid I don't have much time looking into the docs and adding the DBus infrastructure to virtiofsd. If you could help that will be great! Thanks! Eryu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-26 9:18 ` Eryu Guan @ 2019-08-27 14:45 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2019-08-27 14:45 UTC (permalink / raw) To: Eryu Guan; +Cc: Marc-André Lureau, virtio-fs, Daniel Berrange [-- Attachment #1: Type: text/plain, Size: 5894 bytes --] On Mon, Aug 26, 2019 at 05:18:25PM +0800, Eryu Guan wrote: > On Fri, Aug 23, 2019 at 11:51:43AM +0100, Stefan Hajnoczi wrote: > > On Thu, Aug 22, 2019 at 04:35:03PM +0100, Daniel P. Berrangé wrote: > > > On Thu, Aug 22, 2019 at 06:33:05PM +0400, Marc-André Lureau wrote: > > > > Hi > > > > > > > > On Thu, Aug 22, 2019 at 6:16 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > > > > > > On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > > > > > > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > > > > > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > > > > > > >> Introduce a file which contains the target log level, and > > > > > > >> current_log_level will be reloaded to the target log level on SIGHUP. > > > > > > >> > > > > > > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > > > > > > >> sandboxing in virtiofsd, this file can only be read/write via > > > > > > >> /proc/<pid>/fd/<fd> for now. > > > > > > >> > > > > > > >> Example usage: > > > > > > >> > > > > > > >> # view current log level > > > > > > >> cat /proc/<pid>/fd/<fd> > > > > > > >> # change log level to debug > > > > > > >> echo debug > /proc/<pid>/fd/<fd> > > > > > > >> kill -HUP <pid> > > > > > > >> > > > > > > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > > > > > > > > > > > This looks really complex for something that can only be used for > > > > > > > changing log levels. > > > > > > > > > > > > I have the same opinion with Dave, and I wonder if we have some easier > > > > > > channel to tell virtiofsd to change log level. And making the channel > > > > > > more generic is good for the expansibility of virtiofsd. > > > > > > > > > > Adding Dan and Marc-André, who have dealt with DBus and QMP. They can > > > > > share their experience. > > > > > > > > > > The long-term concern we should think about is whether vhost-user > > > > > backends should have standardized management commands. They would be > > > > > defined in docs/interop/vhost-user.rst and all backends implementing a > > > > > device type would implement a subset of them (plus additional > > > > > "vendor-specific commands" if they really don't fit into the vhost-user > > > > > specification). > > > > > > > > > > In order to have standard commands we need to choose a standard RPC > > > > > mechanism. I'm pretty sure that DBus is a good choice based on my > > > > > previous discussions with Dan and Marc-André. It is easy to use from > > > > > most programming languages and therefore easier to integrate than a > > > > > custom management channel. > > > > > > > > > > Dan and Marc-André: The first commands we would like to offer in > > > > > virtiofsd are: > > > > > > > > > > get-log-level -> NUM > > > > > Return the current logging threshold level. > > > > > > > > > > set-log-level NUM > > > > > Set the logging threshold level. > > > > > > > > > > If you have time to give a quick overview of how this looks in DBus that > > > > > would be very helpful. The virtiofsd code is written in C and currently > > > > > > > > Well that depends how generic we want the interface to be :) > > > > > > > > https://dbus.freedesktop.org/doc/dbus-api-design.html > > > > > > > > service-name: none, or org.qemu.VhostUser1 or even org.qemu.VirtioFs1 ? > > > > iface: org.qemu.VhostUser1, org.qemu.VirtioFs1 ? > > > > path: /org/qemu/VhostUser1 > > > > property: "LogLevel", readwrite, string. If you follow systemd/syslogd: > > > > "this accepts a numerical log level or the well-known syslog(3) > > > > symbolic names (lowercase): emerg, alert, crit, err, warning, notice, > > > > info, debug." > > > > > > > > Systemd also has "LogTarget" and a bunch of other options/properties > > > > that are settable from command line and at runtime with dbus (see man > > > > page). > > > > > > > > > > > > > does not use glib/gobject/gio heavily, so I think we'd prefer a > > > > > lightweight approach without a lot of gobject boilerplate, if possible. > > > > > > > > If you can afford GIO, I would highly recommend it. Even if you have > > > > to run it in a seperate thread, that may be easier to maintain. > > > > > > > > Otherwise, sdbus or libdbus. > > > > > > sdbus is fairly nice if you absolutely must avoid GIO, but per my > > > other msg I'd strongly recommend just using GIO for C apps. > > > > > > I would strongly recommend staying well away from libdbus. It is > > > a very low level API and has some very unfortunate design choices > > > which especially impact threaded usage. Most widely used DBus impls > > > have dropped / avoided libdbus these days and just impl the wire > > > protocol themselves. > > > > Thanks for the information. > > > > I have just seen the dbus-vmstate patch on qemu-devel and I guess > > vhost-user will eventually use that for live migration state. So we'll > > have DBus code in virtiofsd anyway. > > > > I think this makes the choice to use DBus an easy one. > > > > Eryu Guan: Sorry that your patch series was side-tracked by a discussion > > about adding a management interface to virtiofsd. But I think we should > > Glad to see the community reaches an agreement on this :) > > > start with DBus (based on what Daniel and Marc-André have suggested) so > > that it's easy to add more management commands in the future. Do you > > want to try this out? Otherwise I can try to rework your patch using > > DBus so we can see what it would look like. > > I have zero experience on DBus, and I'm afraid I don't have much time > looking into the docs and adding the DBus infrastructure to virtiofsd. > > If you could help that will be great! Thanks! Okay, I'll give it a try and CC you on the patch so we can discuss whether it's a good way forward for virtiofsd. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly 2019-08-22 14:16 ` Stefan Hajnoczi 2019-08-22 14:33 ` Marc-André Lureau @ 2019-08-22 15:22 ` Daniel P. Berrangé 1 sibling, 0 replies; 11+ messages in thread From: Daniel P. Berrangé @ 2019-08-22 15:22 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, Marc-André Lureau On Thu, Aug 22, 2019 at 03:16:03PM +0100, Stefan Hajnoczi wrote: > On Thu, Aug 22, 2019 at 01:07:18PM +0800, piaojun wrote: > > On 2019/8/21 23:04, Dr. David Alan Gilbert wrote: > > > * Eryu Guan (eguan@linux.alibaba.com) wrote: > > >> Introduce a file which contains the target log level, and > > >> current_log_level will be reloaded to the target log level on SIGHUP. > > >> > > >> The file is set to "/run/virtiofsd/log_level.<pid>". Due to > > >> sandboxing in virtiofsd, this file can only be read/write via > > >> /proc/<pid>/fd/<fd> for now. > > >> > > >> Example usage: > > >> > > >> # view current log level > > >> cat /proc/<pid>/fd/<fd> > > >> # change log level to debug > > >> echo debug > /proc/<pid>/fd/<fd> > > >> kill -HUP <pid> > > >> > > >> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com> > > > > > > This looks really complex for something that can only be used for > > > changing log levels. > > > > I have the same opinion with Dave, and I wonder if we have some easier > > channel to tell virtiofsd to change log level. And making the channel > > more generic is good for the expansibility of virtiofsd. > > Adding Dan and Marc-André, who have dealt with DBus and QMP. They can > share their experience. Back when QMP was first proposed I argued in favour of using a binary protocol similar to libvirt's use of XDR. We're still suffering long term pain from our use of JSON for QMP due to the inability to handle 64-bit integers in a portable manner. In retrospect I really wish that QEMU uses DBus for its machine monitor interface. Since that time the use of DBus has only expanded across the open source software space, and is most definitely not just for desktop apps. It is very compelling - there are convenient APIs to use DBus from any common programming languages. - the wide usage makes it familiar to many developers - there are many tools for helping with debugging - it is easily consumed my mgmt apps like cockpit which use dbus for everything they mange Overall I would recommend DBus over QMP every time. In retrospect I also wish that libvirt's own RPC was using DBus instead of our XDR based protocol. > The long-term concern we should think about is whether vhost-user > backends should have standardized management commands. They would be > defined in docs/interop/vhost-user.rst and all backends implementing a > device type would implement a subset of them (plus additional > "vendor-specific commands" if they really don't fit into the vhost-user > specification). > > In order to have standard commands we need to choose a standard RPC > mechanism. I'm pretty sure that DBus is a good choice based on my > previous discussions with Dan and Marc-André. It is easy to use from > most programming languages and therefore easier to integrate than a > custom management channel. > > Dan and Marc-André: The first commands we would like to offer in > virtiofsd are: > > get-log-level -> NUM > Return the current logging threshold level. > > set-log-level NUM > Set the logging threshold level. > > If you have time to give a quick overview of how this looks in DBus that > would be very helpful. The virtiofsd code is written in C and currently > does not use glib/gobject/gio heavily, so I think we'd prefer a > lightweight approach without a lot of gobject boilerplate, if possible. I'd venture to suggest that avoiding gobject / gio is actually the heavyweight approach, as you will inevitably be re-inventing the wheel to a large extent ! The boilerplate for creating gobject classes these days is not anywhere near as verbose as people might have seen in the past, from looking at much historical application code/docs. IMHO it is more time efficient & less error/crash prone to write C apps using GLib than to stick with plain C [1]. The GDBus APIs provided by GIO are a very nice & easy way to provide DBus services and clients. There's an example here: https://github.com/mikebrady/gdbus-tutorial/tree/master/src note that most of the dbus boilerplate is autogenerated by the gdbus-codegen command, from the DBus service specification, so you only have to focus on writing the interesting code. It goes with this presentation: https://aleksander.es/data/GNOMEASIA2014%20-%20Introduction%20to%20DBus.pdf https://www.youtube.com/watch?v=egj4UMPaylk Regards, Daniel [1] its even more efficient to not use C, but that's a whole other can or worms :-) -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-27 14:45 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-21 8:56 [Virtio-fs] [PATCH] virtiofsd: add support to change log level on fly Eryu Guan 2019-08-21 15:04 ` Dr. David Alan Gilbert 2019-08-22 5:07 ` piaojun 2019-08-22 13:55 ` Stefan Hajnoczi 2019-08-22 14:16 ` Stefan Hajnoczi 2019-08-22 14:33 ` Marc-André Lureau 2019-08-22 15:35 ` Daniel P. Berrangé 2019-08-23 10:51 ` Stefan Hajnoczi 2019-08-26 9:18 ` Eryu Guan 2019-08-27 14:45 ` Stefan Hajnoczi 2019-08-22 15:22 ` Daniel P. Berrangé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.