On Thu 22 Aug 2013 03:15:48 PM CEST, Eric Blake wrote: > On 08/22/2013 06:19 AM, Martin Kletzander wrote: >> Commit a0b6a36f is fixing what commit abfff210 broke, so to avoid having >> to deal with this issue again, herec comes "virsh-uriprecedence". >> >> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> >> --- > >> @@ -235,6 +236,7 @@ test_scripts += \ >> read-bufsiz \ >> read-non-seekable \ >> start \ >> + virsh-uriprecedence \ >> vcpupin \ > > No longer sorted after the rename :) > Oh, rushing the v2 always gets me :( >> +++ b/tests/virsh-uriprecedence >> @@ -0,0 +1,76 @@ >> +#!/bin/sh > > I'm reviewing this for POSIX sh compatibility... > >> +# Create all mock files/directories to avoid permission problems >> +tmphome="$PWD/tmp_home" >> +export XDG_CONFIG_HOME="$tmphome/.config" >> +export XDG_CACHE_HOME="$tmphome/.cache" >> +export XDG_RUNTIME_HOME="XDG_CACHE_HOME" >> + >> +mkdir -p "$XDG_CONFIG_HOME/libvirt" "$XDG_CONFIG_HOME/virsh" >> +mkdir -p "$XDG_CACHE_HOME/libvirt" "$XDG_CACHE_HOME/virsh" >> +mkdir -p "$XDG_RUNTIME_HOME/libvirt" "$XDG_RUNTIME_HOME/virsh" > > Isn't this last line redundant, since XDG_RUNTIME_HOME is equal to > XDG_CACHE_HOME? Also, you could use one 'mkdir -p' to make all the > directories (but shaving processes isn't essential). > Yes, it can. I thought it'll be better for the future and understandability and won't hurt in case there's one 'mkdir -p'. Unfortunately I the haven't used it. >> + >> +printf "uri_default=\"%s\"\n" "$good_uri" >"$XDG_CONFIG_HOME/libvirt/libvirt.conf" > > Style - slightly shorter as printf 'uri_default="%s"\n' ... > Long line - is it worth wrapping with \-newline? > Definitely, I missed this one. > Everything looks portable - I have no objection to this patch as-is, > although if you haven't pushed yet, you can consider tweaking the nits I > pointed out. > Unfortunately, I pushed it with the thought that 2 ACKs should cover anything. However, I'll be more than happy to fix these in a follow-up patch if you think it's worth it. Martin -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list