Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-09-09 Thread Guillem Jover 
On Fri, 2011-08-26 at 21:36:31 +0200, Raphael Hertzog wrote:
 On Fri, 26 Aug 2011, Guillem Jover wrote:
  +static bool
  +alternative_path_needs_update(const char *linkname, const char *filename)

 IMO you could just as well drop it entirely since you call
 xreadlink(linkname, false) (i.e. it can't error out, but it will return
 NULL in case of failure).

Actually I wanted the common semantics for an x* function. I've now
refactored xreadlink to not take a boolean argument. And found out
another instance where the code was expecting xreadlink never to
return on failure. Included on my next push.

  +   linktarget = xreadlink(linkname, false);
 
 There's a real possibility that linktarget is NULL when linkname doesn't
 exist. That's one of the usual case where can_replace_path() will return
 true and where path_needs_update() will thus be called.

Right, the patch was bogus regarding missing files, “make check” found
about it too though.

 That's the only problem I could spot.

I reimplemented it in a cleaner way.

thanks,
guillem



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-26 Thread Salvatore Bonaccorso 
Hey Raphael

On Tue, Aug 16, 2011 at 10:11:03PM +0200, Raphael Hertzog wrote:
 Hi Salvatore,
 
 On Tue, 16 Aug 2011, Salvatore Bonaccorso wrote:
  Attached is a tentative patch trying to solve this: a link should only
  be updated, if it does not point to the right place.
 
 Can you add a test-case for this? I guess that verifying that the inode
 number did not change is a good way to verify that the link did not get
 replaced... see utils/t/100_update_alternatives.t.

Yes, but I still need to do that.

But attached is an updated version for the patch, is this better
suitable so far? Jan, changed the parts you mentioned.

Regards
Salvatore
From b432597c512aa780c72d40403c607fdc829a1e3b Mon Sep 17 00:00:00 2001
From: Salvatore Bonaccorso car...@debian.org
Date: Fri, 26 Aug 2011 08:30:28 +0200
Subject: [PATCH] Update altenrative links only if needed

Update the alternatives symlinks only if needed. In case the link
already point to the correct location, update-alternatives don't update
the symlink. This is helpfull if e.g. /usr is mountet read-only and the
links there are not affected by an update-alternatives change but only
links in writable part (e.g. /etc/alternatives).

Closes: #636700
---
 utils/update-alternatives.c |   24 +++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/utils/update-alternatives.c b/utils/update-alternatives.c
index 8e82bb6..6d1fade 100644
--- a/utils/update-alternatives.c
+++ b/utils/update-alternatives.c
@@ -1651,6 +1651,26 @@ alternative_can_replace_path(const char *linkname)
 	return replace_link;
 }
 
+static bool
+alternative_link_needs_update(const char *linkname, const char *target)
+{
+	char *lntarget;
+	bool update_needed = true;
+	struct stat st;
+
+	if (lstat(linkname, st) == 0) {
+		if (S_ISLNK(st.st_mode)) {
+			lntarget = xreadlink(linkname, false);
+			if (strcmp(target, lntarget) == 0) {
+update_needed = false;
+			}
+			free(lntarget);
+		}
+	}
+
+	return update_needed;
+}
+
 static void
 alternative_prepare_install_single(struct alternative *a, const char *name,
    const char *linkname, const char *file)
@@ -1665,7 +1685,9 @@ alternative_prepare_install_single(struct alternative *a, const char *name,
 	alternative_add_commit_op(a, opcode_mv, fntmp, fn);
 	free(fntmp);
 
-	if (alternative_can_replace_path(linkname)) {
+	if (!alternative_link_needs_update(linkname,fn)) {
+		/* Link already correct, no update required */
+	} else if (alternative_can_replace_path(linkname)) {
 		/* Create alternative link. */
 		xasprintf(fntmp, %s DPKG_TMP_EXT, linkname);
 		checked_rm(fntmp);
-- 
1.7.5.4



signature.asc
Description: Digital signature

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-26 Thread Raphael Hertzog 
Hi,

On Fri, 26 Aug 2011, Salvatore Bonaccorso wrote:
 But attached is an updated version for the patch, is this better
 suitable so far? Jan, changed the parts you mentioned.

Yes, it's better but still two small points remain:

 +static bool
 +alternative_link_needs_update(const char *linkname, const char *target)
 +{
 + char *lntarget;
 + bool update_needed = true;
 + struct stat st;
 +
 + if (lstat(linkname, st) == 0) {
 + if (S_ISLNK(st.st_mode)) {
 + lntarget = xreadlink(linkname, false);
 + if (strcmp(target, lntarget) == 0) {
 + update_needed = false;
 + }
 + free(lntarget);
 + }
 + }

Here you need:
} else if (errno != ENOENT) {
syserr(_(cannot stat file '%s'), linkname);
}

 - if (alternative_can_replace_path(linkname)) {
 + if (!alternative_link_needs_update(linkname,fn)) {
^missing space

 + /* Link already correct, no update required */
 + } else if (alternative_can_replace_path(linkname)) {
   /* Create alternative link. */
   xasprintf(fntmp, %s DPKG_TMP_EXT, linkname);
   checked_rm(fntmp);

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Follow my Debian News ▶ http://RaphaelHertzog.com (English)
  ▶ http://RaphaelHertzog.fr (Français)



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-26 Thread Guillem Jover 
Hi!

On Fri, 2011-08-26 at 08:44:13 +0200, Salvatore Bonaccorso wrote:
 But attached is an updated version for the patch, is this better
 suitable so far? Jan, changed the parts you mentioned.

The other day while I was messing with u-a I prepared the attached patch
which I think is better (but actually pretty close to what you have
now here :). But forgot including it in my push. In any case I'll
include it for my next one.

thanks,
guillem
From db7c96a14d78bf7ee5a6a89c1be39880cb288c5e Mon Sep 17 00:00:00 2001
From: Guillem Jover guil...@debian.org
Date: Fri, 19 Aug 2011 18:07:13 +0200
Subject: [PATCH] u-a: Update alternative links only if they change

There's no point in changing the links to the same target. This also
helps when systems might have a read-only file system mounted, but a
writable database.

Closes: #636700

Based-on-patch-by: Salvatore Bonaccorso car...@debian.org
Signed-off-by: Guillem Jover guil...@debian.org
---
 utils/update-alternatives.c |   25 ++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/utils/update-alternatives.c b/utils/update-alternatives.c
index 8e82bb6..9b4e2a4 100644
--- a/utils/update-alternatives.c
+++ b/utils/update-alternatives.c
@@ -1651,6 +1651,25 @@ alternative_can_replace_path(const char *linkname)
 	return replace_link;
 }
 
+static bool
+alternative_path_needs_update(const char *linkname, const char *filename)
+{
+	char *linktarget;
+	bool update;
+
+	if (opt_force)
+		return true;
+
+	linktarget = xreadlink(linkname, false);
+	if (strcmp(linktarget, filename) == 0)
+		update = false;
+	else
+		update = true;
+	free(linktarget);
+
+	return update;
+}
+
 static void
 alternative_prepare_install_single(struct alternative *a, const char *name,
    const char *linkname, const char *file)
@@ -1665,15 +1684,15 @@ alternative_prepare_install_single(struct alternative *a, const char *name,
 	alternative_add_commit_op(a, opcode_mv, fntmp, fn);
 	free(fntmp);
 
-	if (alternative_can_replace_path(linkname)) {
+	if (!alternative_can_replace_path(linkname)) {
+		warning(_(not replacing %s with a link.), linkname);
+	} else if (alternative_path_needs_update(linkname, fn)) {
 		/* Create alternative link. */
 		xasprintf(fntmp, %s DPKG_TMP_EXT, linkname);
 		checked_rm(fntmp);
 		checked_symlink(fn, fntmp);
 		alternative_add_commit_op(a, opcode_mv, fntmp, linkname);
 		free(fntmp);
-	} else {
-		warning(_(not replacing %s with a link.), linkname);
 	}
 	free(fn);
 }
-- 
1.7.5.4

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-26 Thread Raphael Hertzog 
Hello Guillem,

On Fri, 26 Aug 2011, Guillem Jover wrote:
 The other day while I was messing with u-a I prepared the attached patch
 which I think is better (but actually pretty close to what you have
 now here :). But forgot including it in my push. In any case I'll
 include it for my next one.

Hum, I think I found a bug in your code. And some details too:

 +static bool
 +alternative_path_needs_update(const char *linkname, const char *filename)
 +{
 + char *linktarget;
 + bool update;
 +
 + if (opt_force)
 + return true;

This early-return clause doesn't make sense, it seems to be there only
because you do no want to run xreadlink() on something that might not be a
symlink. If that's the case, a comment explaining it would be good.

Unless you want --force to redo something that's not needed, but I don't
really see why you'd want that.

IMO you could just as well drop it entirely since you call
xreadlink(linkname, false) (i.e. it can't error out, but it will return
NULL in case of failure).

 + linktarget = xreadlink(linkname, false);

There's a real possibility that linktarget is NULL when linkname doesn't
exist. That's one of the usual case where can_replace_path() will return
true and where path_needs_update() will thus be called.

 + if (strcmp(linktarget, filename) == 0)

Thus you should protect the call here:
if (linktarget  strcmp(…) == 0)

 + update = false;
 + else
 + update = true;
 + free(linktarget);
 +
 + return update;
 +}

That's the only problem I could spot.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Follow my Debian News ▶ http://RaphaelHertzog.com (English)
  ▶ http://RaphaelHertzog.fr (Français)



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-16 Thread Salvatore Bonaccorso 
tag 636700 + patch
thanks

Hi dpkg Team!

On Fri, Aug 05, 2011 at 02:18:58PM +0200, Salvatore Bonaccorso wrote:
 Indeed in the above setup, /usr/lib/nvidia does not have to be
 touched, as it is already correctly pointing to
 /etc/alternatives/nvidia. It would thus be nice, to not update
 symlinks in this case, when they are already correct.

Attached is a tentative patch trying to solve this: a link should only
be updated, if it does not point to the right place.

Is this correct, and if you agree, could it be applied?

Regards
Salvatore
From 9752cd68b05a7dde92e549585a50cf85e166681e Mon Sep 17 00:00:00 2001
From: Jan Hacker hack...@ee.ethz.ch
Date: Tue, 16 Aug 2011 11:33:07 +0200
Subject: [PATCH] update-alternatives: Update a symlink only if the update is needed.

update-alternatives: Update a symlink only if the update is needed. If
the symlink is already correct don't try to update the link.
---
 utils/update-alternatives.c |   14 --
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/utils/update-alternatives.c b/utils/update-alternatives.c
index d829a16..f858c84 100644
--- a/utils/update-alternatives.c
+++ b/utils/update-alternatives.c
@@ -1645,7 +1645,9 @@ static void
 alternative_prepare_install_single(struct alternative *a, const char *name,
    const char *linkname, const char *file)
 {
-	char *fntmp, *fn;
+	char *fntmp, *fn, *lntarget;
+	bool alternative_needs_update = true;
+	struct stat st;
 
 	/* Create link in /etc/alternatives. */
 	xasprintf(fntmp, %s/%s DPKG_TMP_EXT, altdir, name);
@@ -1655,7 +1657,15 @@ alternative_prepare_install_single(struct alternative *a, const char *name,
 	alternative_add_commit_op(a, opcode_mv, fntmp, fn);
 	free(fntmp);
 
-	if (alternative_can_replace_path(linkname)) {
+	/* determine if alternative_needs_update, i.e. link already points to correct target */
+	if (lstat(linkname, st) != -1) {
+		lntarget = xreadlink(linkname,false);
+		if (S_ISLNK(st.st_mode)   strcmp(fn, lntarget) == 0) {
+			alternative_needs_update = false;	
+		}
+	}
+
+	if (alternative_can_replace_path(linkname)  alternative_needs_update) {
 		/* Create alternative link. */
 		xasprintf(fntmp, %s DPKG_TMP_EXT, linkname);
 		checked_rm(fntmp);
-- 
1.7.2.5



signature.asc
Description: Digital signature

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-16 Thread Raphael Hertzog 
Hi Salvatore,

On Tue, 16 Aug 2011, Salvatore Bonaccorso wrote:
 Attached is a tentative patch trying to solve this: a link should only
 be updated, if it does not point to the right place.

Can you add a test-case for this? I guess that verifying that the inode
number did not change is a good way to verify that the link did not get
replaced... see utils/t/100_update_alternatives.t.

Also here are a few comments on your patch:

 --- a/utils/update-alternatives.c
 +++ b/utils/update-alternatives.c
 @@ -1645,7 +1645,9 @@ static void
  alternative_prepare_install_single(struct alternative *a, const char *name,
  const char *linkname, const char *file)
  {
 - char *fntmp, *fn;
 + char *fntmp, *fn, *lntarget;
 + bool alternative_needs_update = true;
 + struct stat st;
  
   /* Create link in /etc/alternatives. */
   xasprintf(fntmp, %s/%s DPKG_TMP_EXT, altdir, name);
 @@ -1655,7 +1657,15 @@ alternative_prepare_install_single(struct alternative 
 *a, const char *name,
   alternative_add_commit_op(a, opcode_mv, fntmp, fn);
   free(fntmp);
  
 - if (alternative_can_replace_path(linkname)) {
 + /* determine if alternative_needs_update, i.e. link already points to 
 correct target */
 + if (lstat(linkname, st) != -1) {

Better check == 0 to match with the rest of the code. Also I would
suggest to put this whole check in a small static function for better
readability (similar to alternative_can_replace_path(), maybe
alternative_link_need_update(link, target)).

 + lntarget = xreadlink(linkname,false);

you have to free(lntarget) at some point since this is malloc()ed.
You miss a space after the comma.

 + if (S_ISLNK(st.st_mode)   strcmp(fn, lntarget) == 0) {
 + alternative_needs_update = false;   
 + }
 + }

You need an else for the lstat() check where you should fail with an error
if the error is unexpected (aka if errno != ENOENT).

 +
 + if (alternative_can_replace_path(linkname)  alternative_needs_update) 
 {

There's an else for this check that prints a warning, but we don't want to
print this warning if we skip the link creation because the link is
already ok.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Follow my Debian News ▶ http://RaphaelHertzog.com (English)
  ▶ http://RaphaelHertzog.fr (Français)



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#636700: update-alternatives: Don't update alternative symlinks if they are already correct

2011-08-05 Thread Salvatore Bonaccorso 
Package: dpkg
Version: 1.15.8.11
Severity: normal

Hi

The following situation causes a problem with version of
update-alternatives in Squeeze, which worked in Lenny.

Prerequisites: We have diskless Clients with read-only /usr and
writable /etc. In /usr/lib there are packages installing
/usr/lib/nvidia-280.13 and /usr/lib/nvidia-173.14.30 directories.

Furthermore a symlink /usr/lib/nvidia to /etc/alternatives/nvidia

Via the alternatives system now we choose where to point
/etc/alternatives/nvdia: either to /usr/lib/nvidia-173.14.30 or
/usr/lib/nvidia-280.13.

With the Lenny Version of update-alternatives updating the alternative
works fine even if /usr is mounted read-only. In Squeeze this now
causes update-alternatives to fail with:

# update-alternatives --list nvidia
/usr/lib/nvidia-173.14.30
/usr/lib/nvidia-280.13
# ls -ld /usr/lib/nvidia*
lrwxrwxrwx 1 root root  24 Aug  4 18:20 /usr/lib/nvidia - 
/etc/alternatives/nvidia
drwxr-xr-x 6 root root 512 Aug  4 16:38 /usr/lib/nvidia-173.14.30
drwxr-xr-x 6 root root 512 Aug  4 16:40 /usr/lib/nvidia-280.13
# ls -l /etc/alternatives/nvidia
lrwxrwxrwx 1 root root 22 Aug  5 11:12 /etc/alternatives/nvidia - 
/usr/lib/nvidia-280.13
# update-alternatives --set nvidia /usr/lib/nvidia-173.14.30
update-alternatives: using /usr/lib/nvidia-173.14.30 to provide /usr/lib/nvidia 
(nvidia) in manual mode.
update-alternatives: error: unable to remove /usr/lib/nvidia.dpkg-tmp: 
Read-only file system

[11:31]  buxy but in general --set might have to remove some of the installed 
symlinks if the slaves are not provided by the new choice
[11:31]  buxy and also the new version tends to fix the symlinks on the fly 
when needed
[11:37]  buxy carnil: feel free to open a bug report suggesting that it would 
be nice to not update symlinks when they are already correct

Indeed in the above setup, /usr/lib/nvidia does not have to be
touched, as it is already correctly pointing to
/etc/alternatives/nvidia. It would thus be nice, to not update
symlinks in this case, when they are already correct.

Regards
Salvatore

-- System Information:
Debian Release: wheezy/sid
  APT prefers unstable
  APT policy: (500, 'unstable')
Architecture: amd64 (x86_64)

Kernel: Linux 2.6.32-5-amd64 (SMP w/8 CPU cores)
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)
Shell: /bin/sh linked to /bin/dash



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org