As previously discussed, it is desirable to move libvirt to a model
where we abort-on-OOM, possibly making use of glib2.

This would be good for libvirt in general, but it is bad for a
couple of libvirt addons.

The virt-login-shell setuid program would be ok with abort-on-OOM,
but absolutely can never link to glib2 in a setuid setup.

The NSS module cannot tolerate abort-on-OOM as it is dyn loaded in
to every process on the host, some of which wish to be robust on
OOM.

It is not practical to restrict abort-on-OOM to only the pieces of
code not used by NSS, nor is it practical to conditionally build
with & without abort-on-OOM.

The solution to both these problems is to refactor the code such
that it does not use any common libvirt code. Only direct libc
APIs are permitted, and for the NSS module the yajl library.

For virt-login-shell this refactoring actually makes the entire
solution more pleasant to deal with, so is a win regardless.

For the NSS module, the code is a little less attractive by
using the lower level libc APIs. The need to use the yajl APIs
directly also makes parsing MACs/leases much more verbose. This
is still tolerable though, given the benefit of switching the
other libvirt code to abort-on-OOM.

Daniel P. Berrangé (17):
  tools: fix crash in virt-login-shell if config doesn't exist
  tools: fix double error reporting in virt-login-shell
  tools: rename source for virt-login-shell
  tools: split virt-login-shell into two binaries
  build: drop libvirt setuid library build
  util: get rid of virIsSUID method
  util: simplify virCommand APIs for env passthrough.
  util: get rid of virGetEnv{Allow,Block}SUID functions
  nss: remove use for virDir helper APIs
  nss: remove use for virString helper APIs
  nss: remove use for virFile helper APIs
  nss: refactor code for processing mac addresses
  nss: custom parser for loading .macs file
  nss: custom parser for loading .leases file
  nss: directly use getnameinfo/getaddrinfo
  nss: remove last usages of libvirt headers
  nss: only link to yajl library and nothing else

 .gitignore                      |   1 +
 cfg.mk                          |  25 +-
 config-post.h                   |  54 ----
 configure.ac                    |   3 -
 libvirt.spec.in                 |   1 +
 src/Makefile.am                 | 174 -------------
 src/libvirt-admin.c             |   2 +-
 src/libvirt.c                   |  47 ++--
 src/libvirt_private.syms        |   6 +-
 src/lxc/lxc_process.c           |   2 +-
 src/network/leaseshelper.c      |  14 +-
 src/qemu/qemu_command.c         |   8 +-
 src/qemu/qemu_firmware.c        |   2 +-
 src/remote/remote_driver.c      |  25 +-
 src/rpc/virnetlibsshsession.c   |   2 +-
 src/rpc/virnetsocket.c          |  16 +-
 src/rpc/virnettlscontext.c      |   2 +-
 src/util/virauth.c              |   2 +-
 src/util/vircommand.c           |  48 +---
 src/util/vircommand.h           |   8 +-
 src/util/virfile.c              |   7 +-
 src/util/virlease.c             |   4 +-
 src/util/virlog.c               |  15 +-
 src/util/virsystemd.c           |   8 +-
 src/util/virutil.c              |  48 +---
 src/util/virutil.h              |   4 -
 src/vbox/vbox_XPCOMCGlue.c      |   2 +-
 src/vbox/vbox_common.c          |   2 +-
 tests/commandtest.c             |   8 +-
 tools/Makefile.am               |  43 ++--
 tools/nss/libvirt_nss.c         | 343 ++++++++-----------------
 tools/nss/libvirt_nss.h         |  24 ++
 tools/nss/libvirt_nss_leases.c  | 429 +++++++++++++++++++++++++++++++
 tools/nss/libvirt_nss_leases.h  |  40 +++
 tools/nss/libvirt_nss_macs.c    | 287 +++++++++++++++++++++
 tools/nss/libvirt_nss_macs.h    |  29 +++
 tools/virsh.c                   |   2 +-
 tools/virt-login-shell-helper.c | 439 ++++++++++++++++++++++++++++++++
 tools/virt-login-shell.c        | 421 ++++--------------------------
 tools/vsh.c                     |  12 +-
 40 files changed, 1521 insertions(+), 1088 deletions(-)
 create mode 100644 tools/nss/libvirt_nss_leases.c
 create mode 100644 tools/nss/libvirt_nss_leases.h
 create mode 100644 tools/nss/libvirt_nss_macs.c
 create mode 100644 tools/nss/libvirt_nss_macs.h
 create mode 100644 tools/virt-login-shell-helper.c

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 8/1/19 5:00 PM, Daniel P. Berrangé wrote:
 >
 >

Very nice cleanup, which also makes NSS library smaller in size (I mean 
those .so.2 files).

I've found some mem leaks and mis-alignments, though. Please squash this 
into respective patches:

diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
index a9814cf0dc..7122065b28 100644
--- a/tools/nss/libvirt_nss.c
+++ b/tools/nss/libvirt_nss.c
@@ -527,7 +527,7 @@ aiforaf(const char *name, int af, struct addrinfo 
*pai, struct addrinfo **aip)

          (*aip)->ai_next = res0;
          while ((*aip)->ai_next)
-           *aip = (*aip)->ai_next;
+            *aip = (*aip)->ai_next;

          addrList++;
      }
diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c
index ddd50288d2..48a54d5841 100644
--- a/tools/nss/libvirt_nss_leases.c
+++ b/tools/nss/libvirt_nss_leases.c
@@ -108,7 +108,7 @@ appendAddr(const char *name ATTRIBUTE_UNUSED,
          return 0;
      }

-     if (af != AF_UNSPEC && af != family) {
+    if (af != AF_UNSPEC && af != family) {
          DEBUG("Skipping address which family is %d, %d requested", 
family, af);
          return 0;
      }
@@ -374,7 +374,7 @@ findLeases(const char *file,
          .addrs = addrs,
          .naddrs = naddrs,
      };
-    yajl_handle parser;
+    yajl_handle parser = NULL;
      char line[1024];
      int rv;

@@ -419,6 +419,7 @@ findLeases(const char *file,
          *addrs = NULL;
          *naddrs = 0;
      }
+    yajl_free(parser);
      free(parserState.entry.ipaddr);
      free(parserState.entry.macaddr);
      free(parserState.entry.hostname);
diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c
index 9fe5b83e86..fb5526bd7b 100644
--- a/tools/nss/libvirt_nss_macs.c
+++ b/tools/nss/libvirt_nss_macs.c
@@ -55,8 +55,8 @@ typedef struct {

  static int
  findMACsParserString(void *ctx,
-                    const unsigned char *stringVal,
-                    size_t stringLen)
+                     const unsigned char *stringVal,
+                     size_t stringLen)
  {
      findMACsParser *parser = ctx;

@@ -70,6 +70,7 @@ findMACsParserString(void *ctx,
          if (STRNEQ(parser->key, "domain"))
              return 0;

+        free(parser->entry.name);
          if (!(parser->entry.name = strndup((char *)stringVal, stringLen)))
              return 0;
      } else if (parser->state == FIND_MACS_STATE_ENTRY_MACS) {
@@ -93,8 +94,8 @@ findMACsParserString(void *ctx,

  static int
  findMACsParserMapKey(void *ctx,
-                    const unsigned char *stringVal,
-                    size_t stringLen)
+                     const unsigned char *stringVal,
+                     size_t stringLen)
  {
      findMACsParser *parser = ctx;

@@ -226,7 +227,7 @@ findMACs(const char *file,
          .macs = macs,
          .nmacs = nmacs,
      };
-    yajl_handle parser;
+    yajl_handle parser = NULL;
      char line[1024];
      size_t i;
      int rv;
@@ -276,6 +277,7 @@ findMACs(const char *file,
          *macs = NULL;
          *nmacs = 0;
      }
+    yajl_free(parser);
      for (i = 0; i < parserState.entry.nmacs; i++)
          free(parserState.entry.macs[i]);
      free(parserState.entry.macs);



With that:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list