Optimize XML parsing in loops

lib/fog/libvirt.rb

@@ -2,6 +2,7 @@
 require 'fog/xml'
 require 'fog/json'
 require 'libvirt'
+require 'nokogiri'
 
 require File.expand_path('../libvirt/version', __FILE__)
 

lib/fog/libvirt/models/compute/util/util.rb

@@ -1,19 +1,8 @@
-require 'nokogiri'
 require 'securerandom'
 
 module Fog
   module Libvirt
     module Util
-      def xml_element(xml, path, attribute=nil)
-        xml = Nokogiri::XML(xml)
-        attribute.nil? ? (xml/path).first.text : (xml/path).first[attribute.to_sym]
-      end
-
-      def xml_elements(xml, path, attribute=nil)
-        xml = Nokogiri::XML(xml)
-        attribute.nil? ? (xml/path).map : (xml/path).map{|element| element[attribute.to_sym]}
-      end
-
       def randomized_name
         "fog-#{(SecureRandom.random_number*10E14).to_i.round}"
       end

lib/fog/libvirt/requests/compute/get_node_info.rb

@@ -12,19 +12,25 @@ def get_node_info
             node_hash[param] = client.send(param) rescue nil
           end
           node_hash[:uri] = client.uri
-          xml = client.sys_info rescue nil
-          [:uuid, :manufacturer, :product, :serial].each do |attr|
-            node_hash[attr] = node_attr(attr, xml) rescue nil
-          end if xml
+          if (xml = sys_info)
+            [:uuid, :manufacturer, :product, :serial].each do |attr|
+              element = xml / "sysinfo/system/entry[@name=#{attr}]"
+              node_hash[attr] = element&.text&.strip
+            end
+          end
 
           node_hash[:hostname] = client.hostname
           [node_hash]
         end
 
         private
 
-        def node_attr attr, xml
-          xml_element(xml, "sysinfo/system/entry[@name='#{attr}']").strip
+        def sys_info
+          Nokogiri::XML(client.sys_info)
+        rescue LibvirtError
+          # qemu:///session typically doesn't have permission to retrieve this
+        rescue StandardError
+          # TODO: log this?
         end
       end
 

lib/fog/libvirt/requests/compute/list_domains.rb

@@ -31,41 +31,41 @@
         def domain_display xml
           attrs = {}
           [:type, :port, :password, :listen].each do |element|
-            attrs[element] = xml_element(xml, "domain/devices/graphics",element.to_s) rescue nil
+            attrs[element] = (xml / "domain/devices/graphics/@#{element}").text
           end
-          attrs.reject{|k,v| v.nil? or v == ""}
+          attrs.reject! { |k, v| v.empty? }
         end
 
         def domain_volumes xml
-          xml_elements(xml, "domain/devices/disk/source").map do |element|
+          (xml / "domain/devices/disk/source").map do |element|
             element[:file] || element[:dev] || element[:name]
           end
         end
 
         def boot_order xml
-          xml_elements(xml, "domain/os/boot", "dev")
+          (xml / "domain/os/boot/@dev").map(&:text)
         end
 
         def firmware(xml)
-          firmware_from_loader = xml_elements(xml, "domain/os/loader", "type").first
+          firmware_from_loader = (xml / "domain/os/loader/@type").text
 
           case firmware_from_loader
           when 'pflash'
             'efi'
           when 'rom'
             'bios'
           else
-            xml_elements(xml, "domain/os", "firmware").first || 'bios'
+            (xml / "domain/os/@firmware").first&.text || 'bios'
           end
         end
 
         # we rely on the fact that the secure attribute is only present when secure boot is enabled
         def secure_boot_enabled?(xml)
-          xml_elements(xml, "domain/os/loader", "secure").first == 'yes'
+          (xml / "domain/os/loader/@secure").text == 'yes'
         end
 
         def domain_interfaces xml
-          ifs = xml_elements(xml, "domain/devices/interface")
+          ifs = xml / "domain/devices/interface"
           ifs.map { |i|
             nics.new({
               :type    => i['type'],
@@ -77,9 +77,11 @@
           }
         end
 
         def domain_to_attributes(dom)
           states= %w(nostate running blocked paused shutting-down shutoff crashed pmsuspended)
 
+          xml = Nokogiri::XML(dom.xml_desc)
+
           begin
             {
               :id              => dom.uuid,
@@ -92,13 +94,13 @@
               :autostart       => dom.autostart?,
               :os_type         => dom.os_type,
               :active          => dom.active?,
-              :display         => domain_display(dom.xml_desc),
-              :boot_order      => boot_order(dom.xml_desc),
-              :nics            => domain_interfaces(dom.xml_desc),
-              :volumes_path    => domain_volumes(dom.xml_desc),
+              :display         => domain_display(xml),
+              :boot_order      => boot_order(xml),
+              :nics            => domain_interfaces(xml),
+              :volumes_path    => domain_volumes(xml),
               :state           => states[dom.info.state],
-              :firmware        => firmware(dom.xml_desc),
-              :secure_boot     => secure_boot_enabled?(dom.xml_desc),
+              :firmware        => firmware(xml),
+              :secure_boot     => secure_boot_enabled?(xml),
             }
           rescue ::Libvirt::RetrieveError, ::Libvirt::Error
             # Catch libvirt exceptions to avoid race conditions involving

lib/fog/libvirt/requests/compute/list_volumes.rb

@@ -25,7 +25,8 @@ def list_volumes(filter = { })
         private
 
         def volume_to_attributes(vol)
-          format_type = xml_element(vol.xml_desc, "/volume/target/format", "type") rescue nil # not all volumes have types, e.g. LVM
+          xml = Nokogiri::XML(vol.xml_desc)
+          format_type = (xml / "/volume/target/format/@type").first&.text&.strip
           return nil if format_type == "dir"
 
           begin

lib/fog/libvirt/requests/compute/update_display.rb

@@ -2,7 +2,7 @@
   module Libvirt
     class Compute
       module Shared
         def update_display(options = { })
           raise ArgumentError, "uuid is a required parameter" unless options.key? :uuid
 
           domain = client.lookup_domain_by_uuid(options[:uuid])
@@ -13,7 +13,7 @@
           display[:listen] = options[:listen].to_s   if options[:listen]
           display[:passwd] = options[:password].to_s if options[:password]
           display[:autoport] = 'yes' if display[:port] == '-1'
-          new_keymap       = options[:keymap] || xml_elements(domain.xml_desc, "graphics", "keymap")[0]
+          new_keymap       = options.fetch(:keymap) { (Nokogiri::XML(domain.xml_desc) / "graphics/@keymap").first&.text }
           display[:keymap] = new_keymap unless new_keymap.nil?
 
           builder = Nokogiri::XML::Builder.new { graphics_ (display) }

tests/libvirt/requests/compute/get_node_info_tests.rb

@@ -0,0 +1,14 @@
+Shindo.tests("Fog::Compute[:libvirt] | get_node_info", 'libvirt') do
+
+  compute = Fog::Compute[:libvirt]
+
+  tests("get_node_info response") do
+    response = compute.get_node_info
+    info = response[0]
+    tests("sys_info attributes") do
+      [:uuid, :manufacturer, :product, :serial].each do |attr|
+        test("attribute #{attr} is set") { info[attr].is_a?(String) }
+      end
+    end
+  end
+end