Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(94)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 5929001: Fixed image_to_live so that it will work with the new sudo (Closed)

Created:
10 years ago by diandersAtChromium
Modified:
9 years, 6 months ago
CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa
Visibility:
Public.

Description

Fixed image_to_live so that it will work with the new sudo This change will not work properly unless the chroot sudo is updated to handle escaping properly: http://codereview.chromium.org/5923001 ...and the chroot sudo will break image_to_live unless this change goes in. Thus, the two changes are intertwined. Change-Id: Ib1602ee47178fc5d32c286e9a914111c1140252b BUG=chromium-os:7072 TEST=Valided that the dev server seemed to be getting proper args. Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=7361be5

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M image_to_live.sh View 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
diandersAtChromium
This needs to be pushed at the same time as: http://codereview.chromium.org/5923001/ ...see that issue for ...
10 years ago (2010-12-15 21:54:37 UTC) #1
anush
Can we check a "sudo -V" output and call the right command ? We can ...
10 years ago (2010-12-15 22:06:01 UTC) #2
diandersAtChromium
Anush, I can do this, but it's not quite as pretty as it seems at ...
10 years ago (2010-12-15 22:52:28 UTC) #3
anush
On Wed, Dec 15, 2010 at 2:52 PM, Doug Anderson <dianders@chromium.org> wrote: > Anush, > ...
10 years ago (2010-12-15 22:56:11 UTC) #4
diandersAtChromium
Hmmm, How about this one (ugly as sin, but...): sudo grep -q '1\.7\.2p2' ../../chroot/usr/bin/sudo -Doug ...
10 years ago (2010-12-15 23:09:26 UTC) #5
diandersAtChromium
10 years ago (2010-12-15 23:30:56 UTC) #6 
Talked with Anush verbally.  He's suggesting pushing the patch as is and not
doing the grep.

For the curious, this was my WIP grep patch:

diff --git a/image_to_live.sh b/image_to_live.sh
index 6c23782..40dc0d4 100755
--- a/image_to_live.sh
+++ b/image_to_live.sh
@@ -153,10 +153,22 @@ function start_dev_server {
       --src_image=\"$(reinterpret_path_for_chroot ${FLAGS_src_image})\""
 
   info "Starting devserver with flags ${devserver_flags}"
-  ./enter_chroot.sh -- sudo sh -c "./start_devserver ${devserver_flags} \
-       --client_prefix=ChromeOSUpdateEngine \
-       --board=${FLAGS_board} \
-       --port=${FLAGS_devserver_port} > ${FLAGS_server_log} 2>&1" &
+
+  # Ugly hack: check if new sudo landed.  Remove 30 days from 2010-12-15.
+  if sudo grep -q '1\.7\.2p4' ../../chroot/usr/bin/sudo; then
+    # Chroot has old sudo.  Run old command...
+    warn "Working around old sudo.  http://codereview.chromium.org/5923001/"
+    ./enter_chroot.sh "sudo ./start_devserver ${devserver_flags} \
+         --client_prefix=ChromeOSUpdateEngine \
+         --board=${FLAGS_board} \
+         --port=${FLAGS_devserver_port} > ${FLAGS_server_log} 2>&1" &
+  else
+    # Chroot has new sudo
+    ./enter_chroot.sh -- sudo sh -c "./start_devserver ${devserver_flags} \
+         --client_prefix=ChromeOSUpdateEngine \
+         --board=${FLAGS_board} \
+         --port=${FLAGS_devserver_port} > ${FLAGS_server_log} 2>&1" &
+  fi
 
   info "Waiting on devserver to start"
   info "note: be patient as the server generates the update before starting."


On 2010/12/15 23:09:26, diandersAtChromium wrote:
> Hmmm,
> 
> How about this one (ugly as sin, but...):
>   sudo grep -q '1\.7\.2p2' ../../chroot/usr/bin/sudo
> 
> -Doug
> 
> ---
> 
> 2010/12/15 Anush Elangovan(அனுஷ்) <mailto:anush@chromium.org>
> 
> > On Wed, Dec 15, 2010 at 2:52 PM, Doug Anderson
<mailto:dianders@chromium.org>
> > wrote:
> > > Anush,
> > > I can do this, but it's not quite as pretty as it seems at first glance.
> > >  Specifically, I need to enter the chroot to check the version of sudo,
> > then
> > > enter the chroot a second time for real.
> > > Hmmm, I could also just run the chroot's sudo myself.  This seems to
> > work,
> > > though it does seem slightly iffy:
> > >     ../../chroot/usr/bin/sudo -V
> > >
> > > Would this be OK?
> >
> > That may not work since there may be differing dependencies on the
> > host. Maybe we will just land the changes together when the builders
> > are already building.
> >
> > LGTM as is.
> >
> > > -Doug
> > > ---
> > > 2010/12/15 Anush Elangovan(அனுஷ்) <mailto:anush@chromium.org>
> > >>
> > >> Can we check a "sudo -V" output and call the right command ? We can
> > >> remove this check after the sudo version has been updated maybe after
> > >> a week.
> > >>
> > >> Thanks
> > >>
> > >> On Wed, Dec 15, 2010 at 1:54 PM,  <mailto:dianders@chromium.org> wrote:
> > >> > Reviewers: anush, djkurtz, davidjames, Mandeep Singh Baines, scottz,
> > >> >
> > >> > Message:
> > >> > This needs to be pushed at the same time as:
> > >> >  http://codereview.chromium.org/5923001/
> > >> >
> > >> > ...see that issue for more details.
> > >> >
> > >> >
> > >> > This particular fix uses "sh -c" to get something similar to the old
> > >> > behavior.
> > >> > That's the only way that I know of to get the redirection to happen
> > >> > properly
> > >> > inside the chroot.
> > >> >
> > >> > Description:
> > >> > Fixed image_to_live so that it will work with the new sudo
> > >> >
> > >> > This change will not work properly unless the chroot sudo is
> > >> > updated to handle escaping properly:
> > >> >  http://codereview.chromium.org/5923001
> > >> >
> > >> > ...and the chroot sudo will break image_to_live unless this
> > >> > change goes in.  Thus, the two changes are intertwined.
> > >> >
> > >> > Change-Id: Ib1602ee47178fc5d32c286e9a914111c1140252b
> > >> >
> > >> > BUG=chromium-os:7072
> > >> > TEST=Valided that the dev server seemed to be getting proper args.
> > >> >
> > >> > Please review this at http://codereview.chromium.org/5929001/
> > >> >
> > >> > SVN Base: ssh://git@gitrw.chromium.org:9222/crosutils.git@master
> > >> >
> > >> > Affected files:
> > >> >  M image_to_live.sh
> > >> >
> > >> >
> > >> > Index: image_to_live.sh
> > >> > diff --git a/image_to_live.sh b/image_to_live.sh
> > >> > index
> > >> >
> > >> >
> >
>
545e428b9281854cb9bac41e154cf2a100a0a3da..6c237820b5f227b1eeca170ec552b0108d54b8cd
> > >> > 100755
> > >> > --- a/image_to_live.sh
> > >> > +++ b/image_to_live.sh
> > >> > @@ -153,7 +153,7 @@ function start_dev_server {
> > >> >       --src_image=\"$(reinterpret_path_for_chroot
> > ${FLAGS_src_image})\""
> > >> >
> > >> >   info "Starting devserver with flags ${devserver_flags}"
> > >> > -  ./enter_chroot.sh "sudo ./start_devserver ${devserver_flags} \
> > >> > +  ./enter_chroot.sh -- sudo sh -c "./start_devserver
> > ${devserver_flags}
> > >> > \
> > >> >        --client_prefix=ChromeOSUpdateEngine \
> > >> >        --board=${FLAGS_board} \
> > >> >        --port=${FLAGS_devserver_port} > ${FLAGS_server_log} 2>&1" &
> > >> > @@ -217,11 +217,11 @@ function get_update_args {
> > >> >  function get_devserver_url {
> > >> >   local devserver_url=""
> > >> >   local port=${FLAGS_devserver_port}
> > >> > -
> > >> > +
> > >> >   if [[ -n ${FLAGS_proxy_port} ]]; then
> > >> >     port=${FLAGS_proxy_port}
> > >> >   fi
> > >> > -
> > >> > +
> > >> >   if [ ${FLAGS_ignore_hostname} -eq ${FLAGS_TRUE} ]; then
> > >> >     if [ -z ${FLAGS_update_url} ]; then
> > >> >       devserver_url="http://$(get_hostname):${port}/update"
> > >> >
> > >> >
> > >> >
> > >
> > >
> >

Powered by Google App Engine
This is Rietveld 408576698