Re: [ntdll] Move file attribute logic to new function DIR_get_attributes()

[Juan Lang]

Hi Dan,
> This is a bit different from the code I posted earlier;
> following Samba's example, it combines the
> fd and filename cases into a single function,

Since you don't actually use the fd in your patch, I think it's
preferred style not to include it for now.

Also,
+void DIR_set_attributes(int fd, DWORD attributes)
+{
+    /* Unimplemented.  Wine does not yet store win32 file attributes. */
(snip)
+
+                    /* Try to save attributes.  We don't care too
much if it doesn't work. */
+                    DIR_set_attributes(fd, info->FileAttributes);

Adding a no-op part of the patch is a bit ugly.  Removing it allows
you to focus on one thing in this patch, rewriting querying the file
attributes.  You re-add setting them later.

(Thanks for working on this, by the way.)
--Juan

[ntdll] Move file attribute logic to new function DIR_get_attributes()

[Dan Kegel]

http://bugs.winehq.org/show_bug.cgi?id=15679 has the latest draft
of the patch.  AJ's probably getting tired of reviewing it
over and over again, so if anyone else can do a quick review
I'd appreciate it.   I'll probably send it in Sunday evening

[ntdll] Move file attribute logic to new function DIR_get_attributes()

[Juan Lang]

Hi Dan,

in patch 2/3,

+    TRACE("fd %d, name %s, attrib %x\n", fd, debugstr_a(unix_fname), *pattrib);
+    *pattrib = attributes;

Any reason you're tracing *pattrib before you set it?

+ * If unix_fname is not NULL, DIR_is_hidden_file is called to do .dotfile check
This is a change from the existing code, and I don't know whether it's
intended or not:

@@ -1575,17 +1575,9 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE
hFile, PIO_STATUS_BLOCK io,
     case FileBasicInformation:
         {
             FILE_BASIC_INFORMATION *info = ptr;
-
-            if (fstat( fd, &st ) == -1)
-                io->u.Status = FILE_GetNtStatus();
-            else if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode))
-                io->u.Status = STATUS_INVALID_INFO_CLASS;
-            else
+            io->u.Status = DIR_get_attributes(fd, NULL,
&info->FileAttributes, &st);

Neither this case, nor the FileAllInformation case, were checking
show_dot_files before.

Also, I'm not sure if this is a useful comment or not, but patch 2/3
is the hardest to review, as every path that was changed needs to be
checked for consistency.  I know that for patch 3, it makes the most
sense for the attribute setting code to be in one location, so I
understand that you want to get there.  Still, it seems possible to
break this one up further.  Part of the patch deals with getting a
struct stat for a file, and the other part deals with deciding a
file's attributes based on a combination of its name and a struct
stat.
--Juan

[ntdll] Move file attribute logic to new function DIR_get_attributes()

[Dan Kegel]

On Sun, Oct 4, 2009 at 9:21 AM, Juan Lang <juan.lang(a)gmail.com> wrote:
> +    TRACE("fd %d, name %s, attrib %x\n", fd, debugstr_a(unix_fname), *pattrib);
> +    *pattrib = attributes;
>
> Any reason you're tracing *pattrib before you set it?

Fixed, thanks!
> + * If unix_fname is not NULL, DIR_is_hidden_file is called to do .dotfile check
> This is a change from the existing code, and I don't know whether it's
> intended or not:...
> Neither this case, nor the FileAllInformation case, were checking
> show_dot_files before.

I'm still not, as in those two cases, unix_fname is NULL.
> Also, I'm not sure if this is a useful comment or not, but patch 2/3
> is the hardest to review... it seems possible to
> break this one up further.

Done, you bastard :-)  I agree, that should be easier to review.

Ready for another look (see latest four attachments to
http://bugs.winehq.org/show_bug.cgi?id=15679 )

Thanks,
Dan

[ntdll] Move file attribute logic to new function DIR_get_attributes()

[Juan Lang]

Hi Dan,

You get a +1 from me on this one.  Small note:
+        if (io->Information == FILE_CREATED) {
+            /* FIXME: is this an extra server round trip? */
(snip)
+            status = server_get_unix_fd( *handle, FILE_WRITE_DATA,
&fd, &needs_close, NULL, NULL );

It is, but for most file operations, it won't have any impact:
they'll call server_get_unix_fd anyway, and the handle will have been
cached.  You could return the fd in the reply if you want to save the
round-trip.

A question, though, and one better answered by Alexandre I'm sure:  is
wineserver a better place for setting and querying the attributes than
ntdll?  I know both have access to the file, so to a certain extent
the difference is immaterial.  On the other hand, since you're already
passing the attributes to the server in the create_file request, it
could handle them there.

The advantage to this is that the tests would pass whether or not
xattr support is present on the tested system, because the attributes
would be stored for the lifetime of the handle.  They wouldn't persist
after closing the handle, of course, and for that you would need xattr
support.
--Juan