Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Michael Albinus]

Po Lu via Mailing list for Emacs changes <emacs-diffs@gnu.org> writes:

Hi,

>     Fix auto-revert-mode on Android
>
>     * src/inotify.c (Finotify_add_watch): Handle asset files.
> ---
>  src/inotify.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/inotify.c b/src/inotify.c
> index 7562ffb1701..b2a48884efa 100644
> --- a/src/inotify.c
> +++ b/src/inotify.c
> @@ -419,6 +419,7 @@ IN_ONESHOT  */)
>    int wd = -1;
>    uint32_t imask = aspect_to_inotifymask (aspect);
>    uint32_t mask = imask | IN_MASK_ADD | IN_EXCL_UNLINK;
> +  char *name;
>
>    CHECK_STRING (filename);
>
> @@ -432,7 +433,23 @@ IN_ONESHOT  */)
>      }
>
>    encoded_file_name = ENCODE_FILE (filename);
> -  wd = inotify_add_watch (inotifyfd, SSDATA (encoded_file_name), mask);
> +  name = SSDATA (encoded_file_name);
> +
> +#if defined HAVE_ANDROID && !defined ANDROID_STUBIFY
> +  /* If FILENAME actually lies in a special directory, return now
> +     instead of letting inotify fail.  These directories cannot
> +     receive file notifications as they are read only.  */
> +
> +  if (strcmp (name, "/assets")
> +      || strcmp (name, "/assets/")
> +      || strcmp (name, "/content")
> +      || strcmp (name, "/content/")
> +      || strncmp (name, "/assets/", sizeof "/assets")
> +      || strncmp (name, "/content", sizeof "/content"))
> +    return Qnil;
> +#endif
> +
> +  wd = inotify_add_watch (inotifyfd, name, mask);
>    if (wd < 0)
>      report_file_notify_error ("Could not add watch for file", filename);

I have two problems with this patch. First, nil is not defined as return
value of *-add-watch functions. If we want to allow this, we must extend
the interface.

Second, do we really want to have such a hard coded list in inotify.c?
What about other directories on other systems, like "/proc"? Shouldn't
we add rather a common interface for excluding directories from being
watched, say file-notify-excluded-directories (a list of strings)? This
could be modified on Lisp level, no need to do it in the notification
backends.

Best regards, Michael.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> Po Lu via Mailing list for Emacs changes <emacs-diffs@gnu.org> writes:
>
> Hi,
>
>>     Fix auto-revert-mode on Android
>>
>>     * src/inotify.c (Finotify_add_watch): Handle asset files.
>> ---
>>  src/inotify.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/inotify.c b/src/inotify.c
>> index 7562ffb1701..b2a48884efa 100644
>> --- a/src/inotify.c
>> +++ b/src/inotify.c
>> @@ -419,6 +419,7 @@ IN_ONESHOT  */)
>>    int wd = -1;
>>    uint32_t imask = aspect_to_inotifymask (aspect);
>>    uint32_t mask = imask | IN_MASK_ADD | IN_EXCL_UNLINK;
>> +  char *name;
>>
>>    CHECK_STRING (filename);
>>
>> @@ -432,7 +433,23 @@ IN_ONESHOT  */)
>>      }
>>
>>    encoded_file_name = ENCODE_FILE (filename);
>> -  wd = inotify_add_watch (inotifyfd, SSDATA (encoded_file_name), mask);
>> +  name = SSDATA (encoded_file_name);
>> +
>> +#if defined HAVE_ANDROID && !defined ANDROID_STUBIFY
>> +  /* If FILENAME actually lies in a special directory, return now
>> +     instead of letting inotify fail.  These directories cannot
>> +     receive file notifications as they are read only.  */
>> +
>> +  if (strcmp (name, "/assets")
>> +      || strcmp (name, "/assets/")
>> +      || strcmp (name, "/content")
>> +      || strcmp (name, "/content/")
>> +      || strncmp (name, "/assets/", sizeof "/assets")
>> +      || strncmp (name, "/content", sizeof "/content"))
>> +    return Qnil;
>> +#endif
>> +
>> +  wd = inotify_add_watch (inotifyfd, name, mask);
>>    if (wd < 0)
>>      report_file_notify_error ("Could not add watch for file", filename);
>
> I have two problems with this patch. First, nil is not defined as return
> value of *-add-watch functions. If we want to allow this, we must extend
> the interface.
>
> Second, do we really want to have such a hard coded list in inotify.c?
> What about other directories on other systems, like "/proc"? Shouldn't
> we add rather a common interface for excluding directories from being
> watched, say file-notify-excluded-directories (a list of strings)? This
> could be modified on Lisp level, no need to do it in the notification
> backends.

Shouldn't watching /proc work, in that no error is signalled, unlike
watching /content and /assets, which are wholly inventions of android.c
which do not exist in the VFS?

Anyway, what return value do you think would be appropriate here?

Thanks.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> I have two problems with this patch. First, nil is not defined as return
>> value of *-add-watch functions. If we want to allow this, we must extend
>> the interface.
>>
>> Second, do we really want to have such a hard coded list in inotify.c?
>> What about other directories on other systems, like "/proc"? Shouldn't
>> we add rather a common interface for excluding directories from being
>> watched, say file-notify-excluded-directories (a list of strings)? This
>> could be modified on Lisp level, no need to do it in the notification
>> backends.
>
> Shouldn't watching /proc work, in that no error is signalled, unlike
> watching /content and /assets, which are wholly inventions of android.c
> which do not exist in the VFS?

I haven't checked /proc, it was just the first example which came to
mind for a directory we might want to exclude. Other examples might be
mounted directories, which could also fail for watching files.

> Anyway, what return value do you think would be appropriate here?

nil might be a correct return value. We haven't simply specified it, and
implementing a file-notify-excluded-directories would give the backends
a simple way to achive the goal, w/o modifying C source.

> Thanks.

Best regards, Michael.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
> Hi,
>
>>> I have two problems with this patch. First, nil is not defined as return
>>> value of *-add-watch functions. If we want to allow this, we must extend
>>> the interface.
>>>
>>> Second, do we really want to have such a hard coded list in inotify.c?
>>> What about other directories on other systems, like "/proc"? Shouldn't
>>> we add rather a common interface for excluding directories from being
>>> watched, say file-notify-excluded-directories (a list of strings)? This
>>> could be modified on Lisp level, no need to do it in the notification
>>> backends.
>>
>> Shouldn't watching /proc work, in that no error is signalled, unlike
>> watching /content and /assets, which are wholly inventions of android.c
>> which do not exist in the VFS?
>
> I haven't checked /proc, it was just the first example which came to
> mind for a directory we might want to exclude. Other examples might be
> mounted directories, which could also fail for watching files.
>
>> Anyway, what return value do you think would be appropriate here?
>
> nil might be a correct return value. We haven't simply specified it, and
> implementing a file-notify-excluded-directories would give the backends
> a simple way to achive the goal, w/o modifying C source.

Hmmm... I'd rather go with modifying C, since the whole point of using
/content and /assets is to be as transparent as possible towards Lisp.
IOW, Lisp shouldn't care.  It should get the same results for /assets
that any other directory gets, except that no file notifications can be
generated, as /assets cannot change.

Thanks.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

> Hmmm... I'd rather go with modifying C, since the whole point of using
> /content and /assets is to be as transparent as possible towards Lisp.
> IOW, Lisp shouldn't care.  It should get the same results for /assets
> that any other directory gets, except that no file notifications can be
> generated, as /assets cannot change.

My fear is that people could get the same idea of adding hard coded file
names to the C level for similar reasons. And/or that this list changes
for future Android versions, requiring to rebuild Emacs.

A configurable Lisp variable would keep this hassle away from us.

> Thanks.

Best regards, Michael.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Po Lu]

Michael Albinus <michael.albinus@gmx.de> writes:

> My fear is that people could get the same idea of adding hard coded file
> names to the C level for similar reasons. And/or that this list changes
> for future Android versions, requiring to rebuild Emacs.

Oh, these file names are not imposed by the system at all -- /assets and
/content only exist because Emacs C code in android.c says so,
explicitly wrapping around non-filesystem APIs in Java.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> My fear is that people could get the same idea of adding hard coded file
>> names to the C level for similar reasons. And/or that this list changes
>> for future Android versions, requiring to rebuild Emacs.
>
> Oh, these file names are not imposed by the system at all -- /assets and
> /content only exist because Emacs C code in android.c says so,
> explicitly wrapping around non-filesystem APIs in Java.

I see. So let it be as-it-is ...

Best regards, Michael.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[chad]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On Sat, Feb 25, 2023 at 7:23 AM Po Lu <luangruo@yahoo.com> wrote:

> Oh, these file names are not imposed by the system at all -- /assets and
> /content only exist because Emacs C code in android.c says so,
> explicitly wrapping around non-filesystem APIs in Java.
>

If these names are strictly by and for emacs, maybe their names should
reflect that somehow? (Sorry to start a naming sub-thread...)

~Chad

[-- Attachment #2: Type: text/html, Size: 779 bytes --]

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[Po Lu]

chad <yandros@gmail.com> writes:

> If these names are strictly by and for emacs, maybe their names should
> reflect that somehow? (Sorry to start a naming sub-thread...)

Changing their names to not look like directories will miss the whole
point of having assets and content providers under directories, so no.

Re: feature/android b91e8ada70e 3/5: Fix auto-revert-mode on Android

[chad]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Sat, Feb 25, 2023 at 9:41 PM Po Lu <luangruo@yahoo.com> wrote:

> chad <yandros@gmail.com> writes:
>
> > If these names are strictly by and for emacs, maybe their names should
> > reflect that somehow? (Sorry to start a naming sub-thread...)
>
> Changing their names to not look like directories will miss the whole
> point of having assets and content providers under directories, so no.
>

I think there is a miscommunication; I intended to suggest that the names
could reflect the fact that they were by and for emacs, not that the names
should not look like directories. Nearly all of my computers have _many_
directories with names that look like directories and are clearly by and
for emacs, so this seemed like a reasonable suggestion. Perhaps there's
some reason unknown to me that these directories need to look like they're
part of general system services (similar to /proc and /mnt, as demonstrated
up-thread); in that case, please pardon the suggestion.

Thanks,
~Chad

[-- Attachment #2: Type: text/html, Size: 1470 bytes --]