)]}'
{"nova/virt/images.py":[{"author":{"_account_id":1812,"name":"p-draigbrady","email":"P@draigBrady.com","username":"p-draigbrady"},"change_message_id":"a13e56ce19852070db48c0bf539b8c5bd6031690","unresolved":false,"context_lines":[{"line_number":168,"context_line":"    if not os.path.exists(path) and CONF.libvirt_images_type !\u003d \u0027rbd\u0027:"},{"line_number":169,"context_line":"        return QemuImgInfo()"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    out, err \u003d utils.execute(\u0027qemu-img\u0027, \u0027info\u0027, path, run_as_root\u003dTrue)"},{"line_number":172,"context_line":"    return QemuImgInfo(out)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAAS3%2F%2FUls%3D","line":171,"updated":"2013-09-30 08:56:29.000000000","message":"This method is called a lot. Do we really need to be root in all cases?\n\nAlso why remove the locale adjustments?\nThat\u0027s required so parsing doesn\u0027t fail if the command outputs translated strings and different number formats.","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"},{"author":{"_account_id":7808,"name":"Xavier Queralt","email":"xqueralt@gmail.com","username":"xqueralt"},"change_message_id":"411c8695f1f53d4c77885aea10827f7f1394c2b0","unresolved":false,"context_lines":[{"line_number":168,"context_line":"    if not os.path.exists(path) and CONF.libvirt_images_type !\u003d \u0027rbd\u0027:"},{"line_number":169,"context_line":"        return QemuImgInfo()"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    out, err \u003d utils.execute(\u0027qemu-img\u0027, \u0027info\u0027, path, run_as_root\u003dTrue)"},{"line_number":172,"context_line":"    return QemuImgInfo(out)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAATH%2F%2F50c%3D","line":171,"in_reply_to":"AAAAS3%2F%2FUls%3D","updated":"2013-10-08 09:23:55.000000000","message":"If we want to make disk images private and do not share groups between nova and qemu/libvirt this method has to be called as root. Having said that, I agree on that libvirt driver is calling this method in excess. Take for example:\n\nhttps://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L1463\nhttps://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4317\n\nIn both cases nova is calling qemu_img_info twice while one would suffice.\n\nOn the other hand, the biggest perpetrator here is libvirt\u0027s get_disk_over_committed_size_total which is called periodically from the update_available_resource periodic task. In this case only the disk size is needed (not the whole qemu_img_info data) which can be obtained directly from libvirt API with the method virDomain.blockInfo.\n\nWould you be fine with qemu-img being called as root if we reduce its usage? I\u0027ll make a separate patch with the changes I mention above.\n\nRegarding the locale adjustements, I removed them because I didn\u0027t want to add env in rootwrap\u0027s filters but I\u0027ve just noticed I can use the EnvFilter for qemu-img which extracts the env in the beginning.","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"},{"author":{"_account_id":1812,"name":"p-draigbrady","email":"P@draigBrady.com","username":"p-draigbrady"},"change_message_id":"b965757097c6c914d3731e887333d004e383d57a","unresolved":false,"context_lines":[{"line_number":168,"context_line":"    if not os.path.exists(path) and CONF.libvirt_images_type !\u003d \u0027rbd\u0027:"},{"line_number":169,"context_line":"        return QemuImgInfo()"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    out, err \u003d utils.execute(\u0027qemu-img\u0027, \u0027info\u0027, path, run_as_root\u003dTrue)"},{"line_number":172,"context_line":"    return QemuImgInfo(out)"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":""}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAATH%2F%2F5uM%3D","line":171,"in_reply_to":"AAAATH%2F%2F50c%3D","updated":"2013-10-08 09:50:24.000000000","message":"Much appreciated for looking into this in such detail.\n\nA patch to reduce calls to qemu_img_info is useful in its own right.\n\nIs it too onerous for nova and libvirt to share groups?\nIsn\u0027t that already the case for well configured systems?\nThis would be very desirable and allow nova to efficiently/securely manipulate these images.\nIf we wanted to support the \"everything as root way\",\nthen perhaps it could be non default like:\n\n    run_as_root \u003d not os.access(path, os.R_OK)","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":1812,"name":"p-draigbrady","email":"P@draigBrady.com","username":"p-draigbrady"},"change_message_id":"a13e56ce19852070db48c0bf539b8c5bd6031690","unresolved":false,"context_lines":[{"line_number":367,"context_line":""},{"line_number":368,"context_line":"        # Change permissions on instances_path so it has the executable flag"},{"line_number":369,"context_line":"        # for others set as needed by libvirt."},{"line_number":370,"context_line":"        libvirt_utils.execute(\u0027chmod\u0027, \u0027751\u0027, CONF.instances_path)"},{"line_number":371,"context_line":""},{"line_number":372,"context_line":"    @property"},{"line_number":373,"context_line":"    def disk_cachemode(self):"}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAAS3%2F%2FUnc%3D","line":370,"updated":"2013-09-30 08:56:29.000000000","message":"All other execute calls are through the \"utils\" module rather than \"libvirt_utils\". Probably best be consistent.  Though saying that, why do we need to have a separate process here? We\u0027re not using rootwrap, so os.chmod() would be much more efficient?","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"},{"author":{"_account_id":7808,"name":"Xavier Queralt","email":"xqueralt@gmail.com","username":"xqueralt"},"change_message_id":"411c8695f1f53d4c77885aea10827f7f1394c2b0","unresolved":false,"context_lines":[{"line_number":367,"context_line":""},{"line_number":368,"context_line":"        # Change permissions on instances_path so it has the executable flag"},{"line_number":369,"context_line":"        # for others set as needed by libvirt."},{"line_number":370,"context_line":"        libvirt_utils.execute(\u0027chmod\u0027, \u0027751\u0027, CONF.instances_path)"},{"line_number":371,"context_line":""},{"line_number":372,"context_line":"    @property"},{"line_number":373,"context_line":"    def disk_cachemode(self):"}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAATH%2F%2F6BU%3D","line":370,"in_reply_to":"AAAAS3%2F%2FUnc%3D","updated":"2013-10-08 09:23:55.000000000","message":"You\u0027re right, os.chmod makes more sense. I will fix this in the next patch.","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"},{"author":{"_account_id":1812,"name":"p-draigbrady","email":"P@draigBrady.com","username":"p-draigbrady"},"change_message_id":"a13e56ce19852070db48c0bf539b8c5bd6031690","unresolved":false,"context_lines":[{"line_number":1398,"context_line":"                out_path \u003d os.path.join(tmpdir, snapshot_name)"},{"line_number":1399,"context_line":"                if live_snapshot:"},{"line_number":1400,"context_line":"                    # NOTE(xqueralt): libvirt needs o+x in the temp directory"},{"line_number":1401,"context_line":"                    utils.execute(\u0027chmod\u0027, \u0027751\u0027, tmpdir)"},{"line_number":1402,"context_line":"                    self._live_snapshot(virt_dom, disk_path, out_path,"},{"line_number":1403,"context_line":"                                        image_format)"},{"line_number":1404,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAAS3%2F%2FUnI%3D","line":1401,"updated":"2013-09-30 08:56:29.000000000","message":"Same comment as above","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"},{"author":{"_account_id":7808,"name":"Xavier Queralt","email":"xqueralt@gmail.com","username":"xqueralt"},"change_message_id":"411c8695f1f53d4c77885aea10827f7f1394c2b0","unresolved":false,"context_lines":[{"line_number":1398,"context_line":"                out_path \u003d os.path.join(tmpdir, snapshot_name)"},{"line_number":1399,"context_line":"                if live_snapshot:"},{"line_number":1400,"context_line":"                    # NOTE(xqueralt): libvirt needs o+x in the temp directory"},{"line_number":1401,"context_line":"                    utils.execute(\u0027chmod\u0027, \u0027751\u0027, tmpdir)"},{"line_number":1402,"context_line":"                    self._live_snapshot(virt_dom, disk_path, out_path,"},{"line_number":1403,"context_line":"                                        image_format)"},{"line_number":1404,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":30,"id":"AAAATH%2F%2F6BM%3D","line":1401,"in_reply_to":"AAAAS3%2F%2FUnI%3D","updated":"2013-10-08 09:23:55.000000000","message":"I will fix this in the next patch.","commit_id":"868f84441ab984bf97479a2fba250c9981d21f56"}],"nova/virt/libvirt/imagebackend.py":[{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"ea5d40683d3a59659a5dd8a83aef1c3d5660b5aa","unresolved":false,"context_lines":[{"line_number":88,"context_line":"        permission bits, so we\u0027ll never accidentally increase permissions."},{"line_number":89,"context_line":"        \"\"\""},{"line_number":90,"context_line":"        # TODO(dripton) remove debug logging"},{"line_number":91,"context_line":"        LOG.debug(\"before user %s uid %s path %s owner %s stat %s\" % ("},{"line_number":92,"context_line":"                  pwd.getpwuid(os.getuid())[0], os.getuid(), path,"},{"line_number":93,"context_line":"                  pwd.getpwuid(os.stat(path).st_uid).pw_name, os.stat(path)))"},{"line_number":94,"context_line":"        cmd \u003d (\u0027chmod\u0027, \u0027-R\u0027, \u0027o-rwx\u0027, path)"}],"source_content_type":"text/x-python","patch_set":24,"id":"AAAAOH%2F%2Fs64%3D","line":91,"updated":"2013-04-02 13:02:30.000000000","message":"You should remove the debugging output, and just add a proper logging.","commit_id":"d8f6949296ce275ec9cc3b7f8107de5f0cf75663"}],"nova/virt/libvirt/utils.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"8885c5d4e623c78c252cdbd460895383d13f650d","unresolved":false,"context_lines":[{"line_number":686,"context_line":"    cmd \u003d (\u0027chgrp\u0027, CONF.libvirt_group, path)"},{"line_number":687,"context_line":"    execute(*cmd, run_as_root\u003dTrue)"},{"line_number":688,"context_line":"    cmd \u003d (\u0027chmod\u0027, \u0027o-rwx,g+rwX\u0027, path)"},{"line_number":689,"context_line":"    execute(*cmd, run_as_root\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":25,"id":"AAAAS3%2F%2Folc%3D","line":689,"updated":"2013-09-23 16:41:11.000000000","message":"I\u0027m not much of a fan of this, since the default setup is not doing anything useful.\n\nNB, libvirt does not require the directory containing images to be owned by the user or group running QEMU.  All that is required is o\u003dx  permission in the directory. eg the default libvirt location has\n\ndrwx--x--x. 2 root root 4096 Sep 19 15:17 /var/lib/libvirt/images\n\nIMHO it would be nicer todo the same for Nova\u0027s instances directory, so we don\u0027t need this new config option.","commit_id":"fe3e91d6b50adcd81925dd5247e5f263db795b81"},{"author":{"_account_id":7808,"name":"Xavier Queralt","email":"xqueralt@gmail.com","username":"xqueralt"},"change_message_id":"0d80010d30139f53585cd0a969ded06ae566b457","unresolved":false,"context_lines":[{"line_number":686,"context_line":"    cmd \u003d (\u0027chgrp\u0027, CONF.libvirt_group, path)"},{"line_number":687,"context_line":"    execute(*cmd, run_as_root\u003dTrue)"},{"line_number":688,"context_line":"    cmd \u003d (\u0027chmod\u0027, \u0027o-rwx,g+rwX\u0027, path)"},{"line_number":689,"context_line":"    execute(*cmd, run_as_root\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":25,"id":"AAAAS3%2F%2FhA4%3D","line":689,"in_reply_to":"AAAAS3%2F%2Folc%3D","updated":"2013-09-25 11:52:12.000000000","message":"Thanks for the tip! I thought libvirt needed the directory to be owned by qemu\u0027s user/group, but you are right, is enough by setting the executable permissions for others.\n\nI\u0027ll push a new patch which uses this approach without the need of a configuration option.","commit_id":"fe3e91d6b50adcd81925dd5247e5f263db795b81"}]}