* [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open @ 2023-07-26 14:59 Thiner Logoer 2023-07-27 13:18 ` David Hildenbrand 0 siblings, 1 reply; 10+ messages in thread From: Thiner Logoer @ 2023-07-26 14:59 UTC (permalink / raw) To: qemu-devel; +Cc: david, imammedo, Thiner Logoer Users may give "-mem-path" a read only file and expect the file to be mapped read-write privately. Allow this but give a warning since other users may surprise when the ram file is readonly and qemu suddenly aborts elsewhere. Suggested-by: David Hildenbrand <david@redhat.com> Signed-off-by: Thiner Logoer <logoerthiner1@163.com> --- See the previous version at: https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ verified, this patch works for my setup, both functionality and the warning are expected behavior. Also another problem when I look at the file_ram_open When readonly is true and the path is a directory, the open will succeed but any later operations will fail since it is a directory fd. This may require additional commits which is out of my scope. Merely record the question here. softmmu/physmem.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 3df73542e1..e8279d69d4 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, char *sanitized_name; char *c; int fd = -1; + bool first_trial = true; *created = false; for (;;) { @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, break; } g_free(filename); + } else if (first_trial && !readonly && errno == EACCES) { + /* @path may be a read only file */ + fd = open(path, O_RDONLY); + if (fd >= 0) { + /* + * Sometimes this behavior is not desired. Fire a warning but + * continue. + */ + warn_report("backing store %s is opened readonly because the" + "file is not writable", path); + break; + } } if (errno != EEXIST && errno != EINTR) { error_setg_errno(errp, errno, @@ -1343,6 +1356,7 @@ static int file_ram_open(const char *path, * Try again on EINTR and EEXIST. The latter happens when * something else creates the file between our two open(). */ + first_trial = false; } return fd; -- 2.40.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-26 14:59 [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open Thiner Logoer @ 2023-07-27 13:18 ` David Hildenbrand 2023-07-27 15:20 ` ThinerLogoer 0 siblings, 1 reply; 10+ messages in thread From: David Hildenbrand @ 2023-07-27 13:18 UTC (permalink / raw) To: Thiner Logoer, qemu-devel; +Cc: imammedo On 26.07.23 16:59, Thiner Logoer wrote: > Users may give "-mem-path" a read only file and expect the file > to be mapped read-write privately. Allow this but give a warning > since other users may surprise when the ram file is readonly and > qemu suddenly aborts elsewhere. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Thiner Logoer <logoerthiner1@163.com> > --- > > See the previous version at: > https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ > > verified, this patch works for my setup, both functionality and the warning > are expected behavior. > > Also another problem when I look at the file_ram_open > > When readonly is true and the path is a directory, the open will succeed but > any later operations will fail since it is a directory fd. This may require > additional commits which is out of my scope. Merely record the question here. > > softmmu/physmem.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 3df73542e1..e8279d69d4 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, > char *sanitized_name; > char *c; > int fd = -1; > + bool first_trial = true; > > *created = false; > for (;;) { > @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, > break; > } > g_free(filename); > + } else if (first_trial && !readonly && errno == EACCES) { I guess it's better to only retry on private mappings, for shared mappings that cannot possibly work. > + /* @path may be a read only file */ > + fd = open(path, O_RDONLY); > + if (fd >= 0) { > + /* > + * Sometimes this behavior is not desired. Fire a warning but > + * continue. > + */ > + warn_report("backing store %s is opened readonly because the" > + "file is not writable", path); Makes sense, this used to not work in the past, now it works with a warning. Now it will work in many cases (except when ftruncate/fallocate would be required, and at least most fallocate callers can handle errors gracefully). -- Cheers, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re:Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-27 13:18 ` David Hildenbrand @ 2023-07-27 15:20 ` ThinerLogoer 2023-07-27 18:30 ` David Hildenbrand 0 siblings, 1 reply; 10+ messages in thread From: ThinerLogoer @ 2023-07-27 15:20 UTC (permalink / raw) To: David Hildenbrand, qemu-devel; +Cc: imammedo At 2023-07-27 21:18:44, "David Hildenbrand" <david@redhat.com> wrote: >On 26.07.23 16:59, Thiner Logoer wrote: >> Users may give "-mem-path" a read only file and expect the file >> to be mapped read-write privately. Allow this but give a warning >> since other users may surprise when the ram file is readonly and >> qemu suddenly aborts elsewhere. >> >> Suggested-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: Thiner Logoer <logoerthiner1@163.com> >> --- >> >> See the previous version at: >> https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ >> >> verified, this patch works for my setup, both functionality and the warning >> are expected behavior. >> >> Also another problem when I look at the file_ram_open >> >> When readonly is true and the path is a directory, the open will succeed but >> any later operations will fail since it is a directory fd. This may require >> additional commits which is out of my scope. Merely record the question here. Maybe you can notice this edge case? I am not sure whether this case is on your todo list? >> >> softmmu/physmem.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >> index 3df73542e1..e8279d69d4 100644 >> --- a/softmmu/physmem.c >> +++ b/softmmu/physmem.c >> @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, >> char *sanitized_name; >> char *c; >> int fd = -1; >> + bool first_trial = true; >> >> *created = false; >> for (;;) { >> @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, >> break; >> } >> g_free(filename); >> + } else if (first_trial && !readonly && errno == EACCES) { > >I guess it's better to only retry on private mappings, for shared >mappings that cannot possibly work. I feel that the retry can be applied in general - for shared mappings, it will merely fail on the mmap step and should be ok? Though, to retry only on private mapping seems straightforwards - this function is called only once, and whether the mapping is private can be passed here with a boolean flag as argument. Nonetheless it may make the logic of the function more complex and less intuitive. --- Regards, logoerthiner ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-27 15:20 ` ThinerLogoer @ 2023-07-27 18:30 ` David Hildenbrand 2023-07-28 4:36 ` ThinerLogoer 2023-07-28 5:46 ` ThinerLogoer 0 siblings, 2 replies; 10+ messages in thread From: David Hildenbrand @ 2023-07-27 18:30 UTC (permalink / raw) To: ThinerLogoer, qemu-devel; +Cc: imammedo On 27.07.23 17:20, ThinerLogoer wrote: > > At 2023-07-27 21:18:44, "David Hildenbrand" <david@redhat.com> wrote: >> On 26.07.23 16:59, Thiner Logoer wrote: >>> Users may give "-mem-path" a read only file and expect the file >>> to be mapped read-write privately. Allow this but give a warning >>> since other users may surprise when the ram file is readonly and >>> qemu suddenly aborts elsewhere. >>> >>> Suggested-by: David Hildenbrand <david@redhat.com> >>> Signed-off-by: Thiner Logoer <logoerthiner1@163.com> >>> --- >>> >>> See the previous version at: >>> https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ >>> >>> verified, this patch works for my setup, both functionality and the warning >>> are expected behavior. >>> >>> Also another problem when I look at the file_ram_open >>> >>> When readonly is true and the path is a directory, the open will succeed but >>> any later operations will fail since it is a directory fd. This may require >>> additional commits which is out of my scope. Merely record the question here. > > Maybe you can notice this edge case? I am not sure whether this > case is on your todo list? I guess we would have to check if we opened a directory. Should be easy to add. As long as QEMU fails reasonably well later, good for now :) > >>> >>> softmmu/physmem.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >>> index 3df73542e1..e8279d69d4 100644 >>> --- a/softmmu/physmem.c >>> +++ b/softmmu/physmem.c >>> @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, >>> char *sanitized_name; >>> char *c; >>> int fd = -1; >>> + bool first_trial = true; >>> >>> *created = false; >>> for (;;) { >>> @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, >>> break; >>> } >>> g_free(filename); >>> + } else if (first_trial && !readonly && errno == EACCES) { >> >> I guess it's better to only retry on private mappings, for shared >> mappings that cannot possibly work. > > I feel that the retry can be applied in general - for shared mappings, > it will merely fail on the mmap step and should be ok? I guess a proper "can't open backing store" message is better for the cases that obviously can't work. > > Though, to retry only on private mapping seems straightforwards - > this function is called only once, and whether the mapping is private > can be passed here with a boolean flag as argument. Nonetheless > it may make the logic of the function more complex and less intuitive. Quick untested attempt to move retry handling to the caller: diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 3df73542e1..c826bb78fc 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd) static int file_ram_open(const char *path, const char *region_name, bool readonly, - bool *created, - Error **errp) + bool *created) { char *filename; char *sanitized_name; @@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path, g_free(filename); } if (errno != EEXIST && errno != EINTR) { - error_setg_errno(errp, errno, - "can't open backing store %s for guest RAM", - path); - return -1; + return -errno; } /* * Try again on EINTR and EEXIST. The latter happens when @@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, bool created; RAMBlock *block; - fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created, - errp); + fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created); + if (fd == -EACCES && !(ram_flags & RAM_SHARED) && readonly) { + /* + * We can have a writable MAP_PRIVATE mapping of a readonly file. + * However, some operations like ftruncate() or fallocate() might fail + * later, let's warn the user. + */ + fd = file_ram_open(mem_path, memory_region_name(mr), true, &created); + if (fd >= 0) { + warn_report("backing store %s for guest RAM (MAP_PRIVATE) opened" + " readonly because the file is not writable", mem_path); + } + } if (fd < 0) { + error_setg_errno(errp, -fd, + "can't open backing store %s for guest RAM", + mem_path); return NULL; } -- 2.41.0 -- Cheers, David / dhildenb ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re:Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-27 18:30 ` David Hildenbrand @ 2023-07-28 4:36 ` ThinerLogoer 2023-07-28 5:46 ` ThinerLogoer 1 sibling, 0 replies; 10+ messages in thread From: ThinerLogoer @ 2023-07-28 4:36 UTC (permalink / raw) To: David Hildenbrand, qemu-devel; +Cc: imammedo At 2023-07-28 02:30:09, "David Hildenbrand" <david@redhat.com> wrote: >On 27.07.23 17:20, ThinerLogoer wrote: >> >> At 2023-07-27 21:18:44, "David Hildenbrand" <david@redhat.com> wrote: >>> On 26.07.23 16:59, Thiner Logoer wrote: >>>> Users may give "-mem-path" a read only file and expect the file >>>> to be mapped read-write privately. Allow this but give a warning >>>> since other users may surprise when the ram file is readonly and >>>> qemu suddenly aborts elsewhere. >>>> >>>> Suggested-by: David Hildenbrand <david@redhat.com> >>>> Signed-off-by: Thiner Logoer <logoerthiner1@163.com> >>>> --- >>>> >>>> See the previous version at: >>>> https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ >>>> >>>> verified, this patch works for my setup, both functionality and the warning >>>> are expected behavior. >>>> >>>> Also another problem when I look at the file_ram_open >>>> >>>> When readonly is true and the path is a directory, the open will succeed but >>>> any later operations will fail since it is a directory fd. This may require >>>> additional commits which is out of my scope. Merely record the question here. >> >> Maybe you can notice this edge case? I am not sure whether this >> case is on your todo list? > >I guess we would have to check if we opened a directory. Should be easy to add. > >As long as QEMU fails reasonably well later, good for now :) > >> >>>> >>>> softmmu/physmem.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >>>> index 3df73542e1..e8279d69d4 100644 >>>> --- a/softmmu/physmem.c >>>> +++ b/softmmu/physmem.c >>>> @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, >>>> char *sanitized_name; >>>> char *c; >>>> int fd = -1; >>>> + bool first_trial = true; >>>> >>>> *created = false; >>>> for (;;) { >>>> @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, >>>> break; >>>> } >>>> g_free(filename); >>>> + } else if (first_trial && !readonly && errno == EACCES) { >>> >>> I guess it's better to only retry on private mappings, for shared >>> mappings that cannot possibly work. >> >> I feel that the retry can be applied in general - for shared mappings, >> it will merely fail on the mmap step and should be ok? > >I guess a proper "can't open backing store" message is better for the cases that obviously can't work. > >> >> Though, to retry only on private mapping seems straightforwards - >> this function is called only once, and whether the mapping is private >> can be passed here with a boolean flag as argument. Nonetheless >> it may make the logic of the function more complex and less intuitive. > >Quick untested attempt to move retry handling to the caller: > >diff --git a/softmmu/physmem.c b/softmmu/physmem.c >index 3df73542e1..c826bb78fc 100644 >--- a/softmmu/physmem.c >+++ b/softmmu/physmem.c >@@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd) > static int file_ram_open(const char *path, > const char *region_name, > bool readonly, >- bool *created, >- Error **errp) >+ bool *created) > { > char *filename; > char *sanitized_name; >@@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path, > g_free(filename); > } > if (errno != EEXIST && errno != EINTR) { >- error_setg_errno(errp, errno, >- "can't open backing store %s for guest RAM", >- path); >- return -1; >+ return -errno; > } > /* > * Try again on EINTR and EEXIST. The latter happens when >@@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, > bool created; > RAMBlock *block; > >- fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created, >- errp); >+ fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created); >+ if (fd == -EACCES && !(ram_flags & RAM_SHARED) && readonly) { >+ /* >+ * We can have a writable MAP_PRIVATE mapping of a readonly file. >+ * However, some operations like ftruncate() or fallocate() might fail >+ * later, let's warn the user. >+ */ >+ fd = file_ram_open(mem_path, memory_region_name(mr), true, &created); >+ if (fd >= 0) { >+ warn_report("backing store %s for guest RAM (MAP_PRIVATE) opened" >+ " readonly because the file is not writable", mem_path); >+ } >+ } > if (fd < 0) { >+ error_setg_errno(errp, -fd, >+ "can't open backing store %s for guest RAM", >+ mem_path); > return NULL; > } > >-- >2.41.0 > > > > >-- >Cheers, > >David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re:Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-27 18:30 ` David Hildenbrand 2023-07-28 4:36 ` ThinerLogoer @ 2023-07-28 5:46 ` ThinerLogoer 2023-07-28 10:45 ` David Hildenbrand 1 sibling, 1 reply; 10+ messages in thread From: ThinerLogoer @ 2023-07-28 5:46 UTC (permalink / raw) To: David Hildenbrand, qemu-devel; +Cc: imammedo Sorry my mail agent just have a bug At 2023-07-28 02:30:09, "David Hildenbrand" <david@redhat.com> wrote: >On 27.07.23 17:20, ThinerLogoer wrote: >> >> At 2023-07-27 21:18:44, "David Hildenbrand" <david@redhat.com> wrote: >>> On 26.07.23 16:59, Thiner Logoer wrote: >>>> Users may give "-mem-path" a read only file and expect the file >>>> to be mapped read-write privately. Allow this but give a warning >>>> since other users may surprise when the ram file is readonly and >>>> qemu suddenly aborts elsewhere. >>>> >>>> Suggested-by: David Hildenbrand <david@redhat.com> >>>> Signed-off-by: Thiner Logoer <logoerthiner1@163.com> >>>> --- >>>> >>>> See the previous version at: >>>> https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ >>>> >>>> verified, this patch works for my setup, both functionality and the warning >>>> are expected behavior. >>>> >>>> Also another problem when I look at the file_ram_open >>>> >>>> When readonly is true and the path is a directory, the open will succeed but >>>> any later operations will fail since it is a directory fd. This may require >>>> additional commits which is out of my scope. Merely record the question here. >> >> Maybe you can notice this edge case? I am not sure whether this >> case is on your todo list? > >I guess we would have to check if we opened a directory. Should be easy to add. > >As long as QEMU fails reasonably well later, good for now :) > >> >>>> >>>> softmmu/physmem.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >>>> index 3df73542e1..e8279d69d4 100644 >>>> --- a/softmmu/physmem.c >>>> +++ b/softmmu/physmem.c >>>> @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, >>>> char *sanitized_name; >>>> char *c; >>>> int fd = -1; >>>> + bool first_trial = true; >>>> >>>> *created = false; >>>> for (;;) { >>>> @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, >>>> break; >>>> } >>>> g_free(filename); >>>> + } else if (first_trial && !readonly && errno == EACCES) { >>> >>> I guess it's better to only retry on private mappings, for shared >>> mappings that cannot possibly work. >> >> I feel that the retry can be applied in general - for shared mappings, >> it will merely fail on the mmap step and should be ok? > >I guess a proper "can't open backing store" message is better for the cases that obviously can't work. > >> >> Though, to retry only on private mapping seems straightforwards - >> this function is called only once, and whether the mapping is private >> can be passed here with a boolean flag as argument. Nonetheless >> it may make the logic of the function more complex and less intuitive. > >Quick untested attempt to move retry handling to the caller: > >diff --git a/softmmu/physmem.c b/softmmu/physmem.c >index 3df73542e1..c826bb78fc 100644 >--- a/softmmu/physmem.c >+++ b/softmmu/physmem.c >@@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd) > static int file_ram_open(const char *path, > const char *region_name, > bool readonly, For some reason this prereq part of patch has one additional space, which causes my `patch` to reject the patch. I have to manually fix it to test later. >- bool *created, >- Error **errp) >+ bool *created) > { > char *filename; > char *sanitized_name; >@@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path, > g_free(filename); > } > if (errno != EEXIST && errno != EINTR) { >- error_setg_errno(errp, errno, >- "can't open backing store %s for guest RAM", >- path); >- return -1; >+ return -errno; > } > /* > * Try again on EINTR and EEXIST. The latter happens when >@@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, > bool created; > RAMBlock *block; > >- fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created, >- errp); >+ fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created); >+ if (fd == -EACCES && !(ram_flags & RAM_SHARED) && readonly) { "readonly" should be "!readonly" here? The readonly variable in this function is about whether the mapping is readonly. In our case the mapping is private writable, so readonly will be false. After manually fix this up, this patch also works in my environment, both functionality and the warning. Maybe you can directly format the patch and start a new thread there? >+ /* >+ * We can have a writable MAP_PRIVATE mapping of a readonly file. >+ * However, some operations like ftruncate() or fallocate() might fail >+ * later, let's warn the user. >+ */ >+ fd = file_ram_open(mem_path, memory_region_name(mr), true, &created); >+ if (fd >= 0) { >+ warn_report("backing store %s for guest RAM (MAP_PRIVATE) opened" >+ " readonly because the file is not writable", mem_path); >+ } >+ } > if (fd < 0) { >+ error_setg_errno(errp, -fd, >+ "can't open backing store %s for guest RAM", >+ mem_path); > return NULL; > } > --- Regards, logoerthiner ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-28 5:46 ` ThinerLogoer @ 2023-07-28 10:45 ` David Hildenbrand 2023-07-29 4:51 ` ThinerLogoer 2023-08-03 15:43 ` Ping: " ThinerLogoer 0 siblings, 2 replies; 10+ messages in thread From: David Hildenbrand @ 2023-07-28 10:45 UTC (permalink / raw) To: ThinerLogoer, qemu-devel; +Cc: imammedo On 28.07.23 07:46, ThinerLogoer wrote: > Sorry my mail agent just have a bug No worries :) > > At 2023-07-28 02:30:09, "David Hildenbrand" <david@redhat.com> wrote: >> On 27.07.23 17:20, ThinerLogoer wrote: >>> >>> At 2023-07-27 21:18:44, "David Hildenbrand" <david@redhat.com> wrote: >>>> On 26.07.23 16:59, Thiner Logoer wrote: >>>>> Users may give "-mem-path" a read only file and expect the file >>>>> to be mapped read-write privately. Allow this but give a warning >>>>> since other users may surprise when the ram file is readonly and >>>>> qemu suddenly aborts elsewhere. >>>>> >>>>> Suggested-by: David Hildenbrand <david@redhat.com> >>>>> Signed-off-by: Thiner Logoer <logoerthiner1@163.com> >>>>> --- >>>>> >>>>> See the previous version at: >>>>> https://lore.kernel.org/qemu-devel/96a462ec-6f9d-fd83-f697-73e132432ca4@redhat.com/T/ >>>>> >>>>> verified, this patch works for my setup, both functionality and the warning >>>>> are expected behavior. >>>>> >>>>> Also another problem when I look at the file_ram_open >>>>> >>>>> When readonly is true and the path is a directory, the open will succeed but >>>>> any later operations will fail since it is a directory fd. This may require >>>>> additional commits which is out of my scope. Merely record the question here. >>> >>> Maybe you can notice this edge case? I am not sure whether this >>> case is on your todo list? >> >> I guess we would have to check if we opened a directory. Should be easy to add. >> >> As long as QEMU fails reasonably well later, good for now :) >> >>> >>>>> >>>>> softmmu/physmem.c | 14 ++++++++++++++ >>>>> 1 file changed, 14 insertions(+) >>>>> >>>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >>>>> index 3df73542e1..e8279d69d4 100644 >>>>> --- a/softmmu/physmem.c >>>>> +++ b/softmmu/physmem.c >>>>> @@ -1296,6 +1296,7 @@ static int file_ram_open(const char *path, >>>>> char *sanitized_name; >>>>> char *c; >>>>> int fd = -1; >>>>> + bool first_trial = true; >>>>> >>>>> *created = false; >>>>> for (;;) { >>>>> @@ -1332,6 +1333,18 @@ static int file_ram_open(const char *path, >>>>> break; >>>>> } >>>>> g_free(filename); >>>>> + } else if (first_trial && !readonly && errno == EACCES) { >>>> >>>> I guess it's better to only retry on private mappings, for shared >>>> mappings that cannot possibly work. >>> >>> I feel that the retry can be applied in general - for shared mappings, >>> it will merely fail on the mmap step and should be ok? >> >> I guess a proper "can't open backing store" message is better for the cases that obviously can't work. >> >>> >>> Though, to retry only on private mapping seems straightforwards - >>> this function is called only once, and whether the mapping is private >>> can be passed here with a boolean flag as argument. Nonetheless >>> it may make the logic of the function more complex and less intuitive. >> >> Quick untested attempt to move retry handling to the caller: >> >> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >> index 3df73542e1..c826bb78fc 100644 >> --- a/softmmu/physmem.c >> +++ b/softmmu/physmem.c >> @@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd) >> static int file_ram_open(const char *path, >> const char *region_name, >> bool readonly, > > For some reason this prereq part of patch has one additional space, > which causes my `patch` to reject the patch. I have to manually > fix it to test later. Yes, to be expected. Pasting a "git show" diff always messes up whitespace for me. It was only meant as a POC. > >> - bool *created, >> - Error **errp) >> + bool *created) >> { >> char *filename; >> char *sanitized_name; >> @@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path, >> g_free(filename); >> } >> if (errno != EEXIST && errno != EINTR) { >> - error_setg_errno(errp, errno, >> - "can't open backing store %s for guest RAM", >> - path); >> - return -1; >> + return -errno; >> } >> /* >> * Try again on EINTR and EEXIST. The latter happens when >> @@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, >> bool created; >> RAMBlock *block; >> >> - fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created, >> - errp); >> + fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created); >> + if (fd == -EACCES && !(ram_flags & RAM_SHARED) && readonly) { > > "readonly" should be "!readonly" here? The readonly variable in this function is > about whether the mapping is readonly. In our case the mapping is private > writable, so readonly will be false. Yes, indeed! > > After manually fix this up, this patch also works in my environment, both > functionality and the warning. > > Maybe you can directly format the patch and start a new thread there? Whatever you prefer! If I resend the patch, I would keep you the author and only add my Co-authored-by: Signed-off-by:. Just let me know. -- Cheers, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re:Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-28 10:45 ` David Hildenbrand @ 2023-07-29 4:51 ` ThinerLogoer 2023-08-03 15:43 ` Ping: " ThinerLogoer 1 sibling, 0 replies; 10+ messages in thread From: ThinerLogoer @ 2023-07-29 4:51 UTC (permalink / raw) To: David Hildenbrand, qemu-devel; +Cc: imammedo At 2023-07-28 18:45:20, "David Hildenbrand" <david@redhat.com> wrote: >>> Quick untested attempt to move retry handling to the caller: >>> >>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c >>> index 3df73542e1..c826bb78fc 100644 >>> --- a/softmmu/physmem.c >>> +++ b/softmmu/physmem.c >>> @@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd) >>> static int file_ram_open(const char *path, >>> const char *region_name, >>> bool readonly, >> >> For some reason this prereq part of patch has one additional space, >> which causes my `patch` to reject the patch. I have to manually >> fix it to test later. > >Yes, to be expected. Pasting a "git show" diff always messes up >whitespace for me. It was only meant as a POC. > >> >>> - bool *created, >>> - Error **errp) >>> + bool *created) >>> { >>> char *filename; >>> char *sanitized_name; >>> @@ -1334,10 +1333,7 @@ static int file_ram_open(const char *path, >>> g_free(filename); >>> } >>> if (errno != EEXIST && errno != EINTR) { >>> - error_setg_errno(errp, errno, >>> - "can't open backing store %s for guest RAM", >>> - path); >>> - return -1; >>> + return -errno; >>> } >>> /* >>> * Try again on EINTR and EEXIST. The latter happens when >>> @@ -1946,9 +1942,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, >>> bool created; >>> RAMBlock *block; >>> >>> - fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created, >>> - errp); >>> + fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created); >>> + if (fd == -EACCES && !(ram_flags & RAM_SHARED) && readonly) { >> >> "readonly" should be "!readonly" here? The readonly variable in this function is >> about whether the mapping is readonly. In our case the mapping is private >> writable, so readonly will be false. > >Yes, indeed! > >> >> After manually fix this up, this patch also works in my environment, both >> functionality and the warning. >> >> Maybe you can directly format the patch and start a new thread there? > > >Whatever you prefer! If I resend the patch, I would keep you the author >and only add my Co-authored-by: Signed-off-by:. > >Just let me know. Everything is good and clear now. I think it is better that you do the patch here. Waiting for the final version of patch. > >-- >Cheers, > >David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Ping: Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-07-28 10:45 ` David Hildenbrand 2023-07-29 4:51 ` ThinerLogoer @ 2023-08-03 15:43 ` ThinerLogoer 2023-08-03 17:17 ` David Hildenbrand 1 sibling, 1 reply; 10+ messages in thread From: ThinerLogoer @ 2023-08-03 15:43 UTC (permalink / raw) To: David Hildenbrand, qemu-devel; +Cc: imammedo At 2023-07-28 18:45:20, "David Hildenbrand" <david@redhat.com> wrote: > > >Whatever you prefer! If I resend the patch, I would keep you the author >and only add my Co-authored-by: Signed-off-by:. > >Just let me know. > Hello, I wonder whether you have planned to resubmit the current patch anytime soon, or is it already inside the patch queue? --- Regards, logoerthiner ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Ping: Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open 2023-08-03 15:43 ` Ping: " ThinerLogoer @ 2023-08-03 17:17 ` David Hildenbrand 0 siblings, 0 replies; 10+ messages in thread From: David Hildenbrand @ 2023-08-03 17:17 UTC (permalink / raw) To: ThinerLogoer, qemu-devel; +Cc: imammedo On 03.08.23 17:43, ThinerLogoer wrote: > At 2023-07-28 18:45:20, "David Hildenbrand" <david@redhat.com> wrote: >> >> >> Whatever you prefer! If I resend the patch, I would keep you the author >> and only add my Co-authored-by: Signed-off-by:. >> >> Just let me know. >> > > Hello, > > I wonder whether you have planned to resubmit the current patch anytime soon, or is it already > inside the patch queue? I'm currently testing the following change on top, not compile-tested under Windows, though. From b1abec2fe024ea90860ecf600c381e4a25e22ed8 Mon Sep 17 00:00:00 2001 From: David Hildenbrand <david@redhat.com> Date: Thu, 3 Aug 2023 19:14:34 +0200 Subject: [PATCH] softmmu/physmem: Always detect and handle directories in file_ram_open() open() does not fail on directories when opening readonly -- O_RDONLY. To identify directories and handle them accordingly in file_ram_open() also when readonly=true was specified, detect if we just opened a directory using fstat() instead. Before this change: $ ./qemu-system-x86_64 \ -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g qemu-system-x86_64: unable to map backing store for guest RAM: No such device With this change, it works as expected: we create a temporary hidden file in that directory, just like when specifying readonly=false. Reported-by: Thiner Logoer <logoerthiner1@163.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- softmmu/physmem.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/softmmu/physmem.c b/softmmu/physmem.c index d1ae694b20..32b51fd54d 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1300,9 +1300,29 @@ static int file_ram_open(const char *path, for (;;) { fd = open(path, readonly ? O_RDONLY : O_RDWR); if (fd >= 0) { - /* @path names an existing file, use it */ - break; + /* + * open() won't fail when passing O_RDONLY on directories. So + * check manually if we're given a directory, and convert to + * EISDIR. + */ + if (readonly) { + struct stat file_stat; + + if (fstat(fd, &file_stat)) { + close(fd); + return -errno; + } else if (S_ISDIR(file_stat.st_mode)) { + close(fd); + fd = -1; + errno = EISDIR; + } + } + if (fd >= 0) { + /* @path names an existing file, use it */ + break; + } } + if (errno == ENOENT) { /* @path names a file that doesn't exist, create it */ fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644); -- 2.41.0 -- Cheers, David / dhildenb ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-08-03 17:18 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-26 14:59 [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open Thiner Logoer 2023-07-27 13:18 ` David Hildenbrand 2023-07-27 15:20 ` ThinerLogoer 2023-07-27 18:30 ` David Hildenbrand 2023-07-28 4:36 ` ThinerLogoer 2023-07-28 5:46 ` ThinerLogoer 2023-07-28 10:45 ` David Hildenbrand 2023-07-29 4:51 ` ThinerLogoer 2023-08-03 15:43 ` Ping: " ThinerLogoer 2023-08-03 17:17 ` David Hildenbrand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).