Re: [PATCH] Improve sftp symlink support

[Alexander Larsson <alexl redhat com>]

On Fri, 2006-03-17 at 22:04 +0100, Christian Neumair wrote:

> The attached patch now also handled NULL and "" paths better by
> canonicalizing them to "/", and it adds more DEBUG statements. Thanks to
> Priit Laes for pointing this glitch out.

+if (symlink[0] == '/' || !strcmp (path, "/"))
+	return g_strdup (symlink);

Why the strcmp? resolve("/", "blah") should be "/blah", not "blah". I
removed this.

	p = strrchr (path, '/');
	g_assert (p != NULL);

An assert is a bit harsh in this place, i made it just return symlink in
this case.


+	if (id != expected_id) {
+		g_critical ("ID mismatch (%u != %u)", id, expected_id);

Don't user g_critical for this, as its perfectly possible to return an
error code. g_critical should mainly be used instead of e.g. crashing
with a null-pointer dereference, it should only happen because the app
did something wrong, not because of something the server sent to us. I
realize the sftp code is full of g_criticals, but we shouldn't propagate
this more.

		} else
			DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG,
				      "%s: Readlink failed, got unexpected filename count (%d, 1
expected)",
				      __FUNCTION__, count));

This doesn't work well if DEBUG isn't defined. You need { } around it.


I fixed up all the above comments and commited it. However there are
still some issues with the symlink semantics:

+	/* we can't use FSTAT, because it always follows the symlink, but doesn't return the target file name.
+	 * So we fall back to our home-brewn LSTAT/readlink code. */
+	return get_file_info_for_path (handle->connection,
+				       handle->path,
+				       file_info, options);

For file: do_get_file_info_from_handle uses fstat, which also always
follows symlinks, so I think that is the right behaviour. We should do
the same in sftp:.

+			if (info->type == GNOME_VFS_FILE_TYPE_SYMBOLIC_LINK) {
+				path = g_build_filename (handle->path, filename, NULL);
+				get_file_info_for_path (handle->connection, path,
+							info, handle->dir_options);
+				g_free (path);
+			} else

This needs to check FOLLOW_SYMLINKS in the options before following the
symlink.

Can you look into this?

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a globe-trotting flyboy shaman with a robot buddy named Sparky. She's a 
chain-smoking French-Canadian journalist descended from a line of powerful 
witches. They fight crime!