* [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
@ 2018-06-01 1:25 ` Keno Fischer
2018-06-01 8:15 ` Greg Kurz
2018-06-01 14:15 ` Eric Blake
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 02/20] 9p: proxy: Fix size passed to `connect` Keno Fischer
` (18 subsequent siblings)
19 siblings, 2 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
strchrnul is a GNU extension and thus unavailable on a number of targets.
In the review for a commit removing strchrnul from 9p, I was asked to
create a qemu_strchrnul helper to factor out this functionality.
Do so, and use it in a number of other places in the code base that inlined
the replacement pattern in a place where strchrnul could be used.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1: New patch
hw/9pfs/9p-local.c | 2 +-
include/qemu/cutils.h | 1 +
monitor.c | 8 ++------
util/cutils.c | 13 +++++++++++++
util/qemu-option.c | 6 +-----
util/uri.c | 6 ++----
6 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b37b1db..bcf2798 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
assert(*path != '/');
head = g_strdup(path);
- c = strchrnul(path, '/');
+ c = qemu_strchrnul(path, '/');
if (*c) {
/* Intermediate path element */
head[c - path] = 0;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index a663340..bc40c30 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
* Returns: the pointer originally in @input.
*/
char *qemu_strsep(char **input, const char *delim);
+const char *qemu_strchrnul(const char *s, int c);
time_t mktimegm(struct tm *tm);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
diff --git a/monitor.c b/monitor.c
index 922cfc0..e1f01c4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -798,9 +798,7 @@ static int compare_cmd(const char *name, const char *list)
p = list;
for(;;) {
pstart = p;
- p = strchr(p, '|');
- if (!p)
- p = pstart + strlen(pstart);
+ p = qemu_strchrnul(p, '|');
if ((p - pstart) == len && !memcmp(pstart, name, len))
return 1;
if (*p == '\0')
@@ -3401,9 +3399,7 @@ static void cmd_completion(Monitor *mon, const char *name, const char *list)
p = list;
for(;;) {
pstart = p;
- p = strchr(p, '|');
- if (!p)
- p = pstart + strlen(pstart);
+ p = qemu_strchrnul(p, '|');
len = p - pstart;
if (len > sizeof(cmd) - 2)
len = sizeof(cmd) - 2;
diff --git a/util/cutils.c b/util/cutils.c
index 0de69e6..6e078b0 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -545,6 +545,19 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
}
/**
+ * Searches for the first occurrence of 'c' in 's', and returns a pointer
+ * to the trailing null byte if none was found.
+ */
+const char *qemu_strchrnul(const char *s, int c)
+{
+ const char *e = strchr(s, c);
+ if (!e) {
+ e = s + strlen(s);
+ }
+ return e;
+}
+
+/**
* parse_uint:
*
* @s: String to parse
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 58d1c23..54eca12 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -77,11 +77,7 @@ const char *get_opt_value(const char *p, char **value)
*value = NULL;
while (1) {
- offset = strchr(p, ',');
- if (!offset) {
- offset = p + strlen(p);
- }
-
+ offset = qemu_strchrnul(p, ',');
length = offset - p;
if (*offset != '\0' && *(offset + 1) == ',') {
length++;
diff --git a/util/uri.c b/util/uri.c
index 8624a7a..8bdef84 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -52,6 +52,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include "qemu/uri.h"
@@ -2266,10 +2267,7 @@ struct QueryParams *query_params_parse(const char *query)
/* Find the next separator, or end of the string. */
end = strchr(query, '&');
if (!end) {
- end = strchr(query, ';');
- }
- if (!end) {
- end = query + strlen(query);
+ end = qemu_strchrnul(query, ';');
}
/* Find the first '=' character between here and end. */
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul Keno Fischer
@ 2018-06-01 8:15 ` Greg Kurz
2018-06-01 8:46 ` Dr. David Alan Gilbert
2018-06-01 14:15 ` Eric Blake
1 sibling, 1 reply; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 8:15 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel, Markus Armbruster, Dr. David Alan Gilbert
On Thu, 31 May 2018 21:25:56 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> strchrnul is a GNU extension and thus unavailable on a number of targets.
> In the review for a commit removing strchrnul from 9p, I was asked to
> create a qemu_strchrnul helper to factor out this functionality.
> Do so, and use it in a number of other places in the code base that inlined
> the replacement pattern in a place where strchrnul could be used.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
And possibly we could detect in configure if the host has strchrnul() and
use it, but this optimization can be done later.
I haven't checked if there could be other candidates in the current code
base though. Also, this patch touches some other subsystems, so I'm Cc'ing
to the other maintainers as reported by ./scripts/get_maintainer.pl:
Greg Kurz <groug@kaod.org> (supporter:virtio-9p)
Markus Armbruster <armbru@redhat.com> (supporter:QMP)
"Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Human Monitor (HMP))
qemu-devel@nongnu.org (open list:All patches CC here)
Anyway,
Acked-by: Greg Kurz <groug@kaod.org>
> Changes since v1: New patch
>
> hw/9pfs/9p-local.c | 2 +-
> include/qemu/cutils.h | 1 +
> monitor.c | 8 ++------
> util/cutils.c | 13 +++++++++++++
> util/qemu-option.c | 6 +-----
> util/uri.c | 6 ++----
> 6 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index b37b1db..bcf2798 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
> assert(*path != '/');
>
> head = g_strdup(path);
> - c = strchrnul(path, '/');
> + c = qemu_strchrnul(path, '/');
> if (*c) {
> /* Intermediate path element */
> head[c - path] = 0;
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index a663340..bc40c30 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
> * Returns: the pointer originally in @input.
> */
> char *qemu_strsep(char **input, const char *delim);
> +const char *qemu_strchrnul(const char *s, int c);
> time_t mktimegm(struct tm *tm);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> diff --git a/monitor.c b/monitor.c
> index 922cfc0..e1f01c4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -798,9 +798,7 @@ static int compare_cmd(const char *name, const char *list)
> p = list;
> for(;;) {
> pstart = p;
> - p = strchr(p, '|');
> - if (!p)
> - p = pstart + strlen(pstart);
> + p = qemu_strchrnul(p, '|');
> if ((p - pstart) == len && !memcmp(pstart, name, len))
> return 1;
> if (*p == '\0')
> @@ -3401,9 +3399,7 @@ static void cmd_completion(Monitor *mon, const char *name, const char *list)
> p = list;
> for(;;) {
> pstart = p;
> - p = strchr(p, '|');
> - if (!p)
> - p = pstart + strlen(pstart);
> + p = qemu_strchrnul(p, '|');
> len = p - pstart;
> if (len > sizeof(cmd) - 2)
> len = sizeof(cmd) - 2;
> diff --git a/util/cutils.c b/util/cutils.c
> index 0de69e6..6e078b0 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -545,6 +545,19 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
> }
>
> /**
> + * Searches for the first occurrence of 'c' in 's', and returns a pointer
> + * to the trailing null byte if none was found.
> + */
> +const char *qemu_strchrnul(const char *s, int c)
> +{
> + const char *e = strchr(s, c);
> + if (!e) {
> + e = s + strlen(s);
> + }
> + return e;
> +}
> +
> +/**
> * parse_uint:
> *
> * @s: String to parse
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 58d1c23..54eca12 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -77,11 +77,7 @@ const char *get_opt_value(const char *p, char **value)
>
> *value = NULL;
> while (1) {
> - offset = strchr(p, ',');
> - if (!offset) {
> - offset = p + strlen(p);
> - }
> -
> + offset = qemu_strchrnul(p, ',');
> length = offset - p;
> if (*offset != '\0' && *(offset + 1) == ',') {
> length++;
> diff --git a/util/uri.c b/util/uri.c
> index 8624a7a..8bdef84 100644
> --- a/util/uri.c
> +++ b/util/uri.c
> @@ -52,6 +52,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>
> #include "qemu/uri.h"
>
> @@ -2266,10 +2267,7 @@ struct QueryParams *query_params_parse(const char *query)
> /* Find the next separator, or end of the string. */
> end = strchr(query, '&');
> if (!end) {
> - end = strchr(query, ';');
> - }
> - if (!end) {
> - end = query + strlen(query);
> + end = qemu_strchrnul(query, ';');
> }
>
> /* Find the first '=' character between here and end. */
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul
2018-06-01 8:15 ` Greg Kurz
@ 2018-06-01 8:46 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 34+ messages in thread
From: Dr. David Alan Gilbert @ 2018-06-01 8:46 UTC (permalink / raw)
To: Greg Kurz; +Cc: Keno Fischer, qemu-devel, Markus Armbruster
* Greg Kurz (groug@kaod.org) wrote:
> On Thu, 31 May 2018 21:25:56 -0400
> Keno Fischer <keno@juliacomputing.com> wrote:
>
> > strchrnul is a GNU extension and thus unavailable on a number of targets.
> > In the review for a commit removing strchrnul from 9p, I was asked to
> > create a qemu_strchrnul helper to factor out this functionality.
> > Do so, and use it in a number of other places in the code base that inlined
> > the replacement pattern in a place where strchrnul could be used.
> >
> > Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> > ---
> >
>
> And possibly we could detect in configure if the host has strchrnul() and
> use it, but this optimization can be done later.
>
> I haven't checked if there could be other candidates in the current code
> base though. Also, this patch touches some other subsystems, so I'm Cc'ing
> to the other maintainers as reported by ./scripts/get_maintainer.pl:
That looks fine from my point of view; I can see you could probably
also use it in the code at the start of the while loop in hmp_sendkey:
while (1) {
separator = strchr(keys, '-');
keyname_len = separator ? separator - keys : strlen(keys);
Dave
> Greg Kurz <groug@kaod.org> (supporter:virtio-9p)
> Markus Armbruster <armbru@redhat.com> (supporter:QMP)
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> (maintainer:Human Monitor (HMP))
> qemu-devel@nongnu.org (open list:All patches CC here)
>
> Anyway,
>
> Acked-by: Greg Kurz <groug@kaod.org>
>
> > Changes since v1: New patch
> >
> > hw/9pfs/9p-local.c | 2 +-
> > include/qemu/cutils.h | 1 +
> > monitor.c | 8 ++------
> > util/cutils.c | 13 +++++++++++++
> > util/qemu-option.c | 6 +-----
> > util/uri.c | 6 ++----
> > 6 files changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index b37b1db..bcf2798 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
> > assert(*path != '/');
> >
> > head = g_strdup(path);
> > - c = strchrnul(path, '/');
> > + c = qemu_strchrnul(path, '/');
> > if (*c) {
> > /* Intermediate path element */
> > head[c - path] = 0;
> > diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> > index a663340..bc40c30 100644
> > --- a/include/qemu/cutils.h
> > +++ b/include/qemu/cutils.h
> > @@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
> > * Returns: the pointer originally in @input.
> > */
> > char *qemu_strsep(char **input, const char *delim);
> > +const char *qemu_strchrnul(const char *s, int c);
> > time_t mktimegm(struct tm *tm);
> > int qemu_fdatasync(int fd);
> > int fcntl_setfl(int fd, int flag);
> > diff --git a/monitor.c b/monitor.c
> > index 922cfc0..e1f01c4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -798,9 +798,7 @@ static int compare_cmd(const char *name, const char *list)
> > p = list;
> > for(;;) {
> > pstart = p;
> > - p = strchr(p, '|');
> > - if (!p)
> > - p = pstart + strlen(pstart);
> > + p = qemu_strchrnul(p, '|');
> > if ((p - pstart) == len && !memcmp(pstart, name, len))
> > return 1;
> > if (*p == '\0')
> > @@ -3401,9 +3399,7 @@ static void cmd_completion(Monitor *mon, const char *name, const char *list)
> > p = list;
> > for(;;) {
> > pstart = p;
> > - p = strchr(p, '|');
> > - if (!p)
> > - p = pstart + strlen(pstart);
> > + p = qemu_strchrnul(p, '|');
> > len = p - pstart;
> > if (len > sizeof(cmd) - 2)
> > len = sizeof(cmd) - 2;
> > diff --git a/util/cutils.c b/util/cutils.c
> > index 0de69e6..6e078b0 100644
> > --- a/util/cutils.c
> > +++ b/util/cutils.c
> > @@ -545,6 +545,19 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
> > }
> >
> > /**
> > + * Searches for the first occurrence of 'c' in 's', and returns a pointer
> > + * to the trailing null byte if none was found.
> > + */
> > +const char *qemu_strchrnul(const char *s, int c)
> > +{
> > + const char *e = strchr(s, c);
> > + if (!e) {
> > + e = s + strlen(s);
> > + }
> > + return e;
> > +}
> > +
> > +/**
> > * parse_uint:
> > *
> > * @s: String to parse
> > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > index 58d1c23..54eca12 100644
> > --- a/util/qemu-option.c
> > +++ b/util/qemu-option.c
> > @@ -77,11 +77,7 @@ const char *get_opt_value(const char *p, char **value)
> >
> > *value = NULL;
> > while (1) {
> > - offset = strchr(p, ',');
> > - if (!offset) {
> > - offset = p + strlen(p);
> > - }
> > -
> > + offset = qemu_strchrnul(p, ',');
> > length = offset - p;
> > if (*offset != '\0' && *(offset + 1) == ',') {
> > length++;
> > diff --git a/util/uri.c b/util/uri.c
> > index 8624a7a..8bdef84 100644
> > --- a/util/uri.c
> > +++ b/util/uri.c
> > @@ -52,6 +52,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> >
> > #include "qemu/uri.h"
> >
> > @@ -2266,10 +2267,7 @@ struct QueryParams *query_params_parse(const char *query)
> > /* Find the next separator, or end of the string. */
> > end = strchr(query, '&');
> > if (!end) {
> > - end = strchr(query, ';');
> > - }
> > - if (!end) {
> > - end = query + strlen(query);
> > + end = qemu_strchrnul(query, ';');
> > }
> >
> > /* Find the first '=' character between here and end. */
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul Keno Fischer
2018-06-01 8:15 ` Greg Kurz
@ 2018-06-01 14:15 ` Eric Blake
1 sibling, 0 replies; 34+ messages in thread
From: Eric Blake @ 2018-06-01 14:15 UTC (permalink / raw)
To: Keno Fischer, qemu-devel; +Cc: groug
On 05/31/2018 08:25 PM, Keno Fischer wrote:
> strchrnul is a GNU extension and thus unavailable on a number of targets.
> In the review for a commit removing strchrnul from 9p, I was asked to
> create a qemu_strchrnul helper to factor out this functionality.
> Do so, and use it in a number of other places in the code base that inlined
> the replacement pattern in a place where strchrnul could be used.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
> +++ b/util/cutils.c
> @@ -545,6 +545,19 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
> }
>
> /**
> + * Searches for the first occurrence of 'c' in 's', and returns a pointer
> + * to the trailing null byte if none was found.
> + */
> +const char *qemu_strchrnul(const char *s, int c)
> +{
> + const char *e = strchr(s, c);
> + if (!e) {
> + e = s + strlen(s);
> + }
> + return e;
> +}
This is twice as slow on glibc systems when the pointer to NUL is
returned (because it has to traverse the string twice); it's better to
have a configure check for whether strchrnul exists, and if so, use that
directly.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 02/20] 9p: proxy: Fix size passed to `connect`
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul Keno Fischer
@ 2018-06-01 1:25 ` Keno Fischer
2018-06-01 9:09 ` Greg Kurz
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 03/20] 9p: xattr: Fix crash due to free of uninitialized value Keno Fischer
` (17 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
The size to pass to the `connect` call is the size of the entire
`struct sockaddr_un`. Passing anything shorter than this causes errors
on darwin.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1: New patch
hw/9pfs/9p-proxy.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index e2e0329..47a94e0 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1088,7 +1088,7 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
static int connect_namedsocket(const char *path, Error **errp)
{
- int sockfd, size;
+ int sockfd;
struct sockaddr_un helper;
if (strlen(path) >= sizeof(helper.sun_path)) {
@@ -1102,8 +1102,7 @@ static int connect_namedsocket(const char *path, Error **errp)
}
strcpy(helper.sun_path, path);
helper.sun_family = AF_UNIX;
- size = strlen(helper.sun_path) + sizeof(helper.sun_family);
- if (connect(sockfd, (struct sockaddr *)&helper, size) < 0) {
+ if (connect(sockfd, (struct sockaddr *)&helper, sizeof(helper)) < 0) {
error_setg_errno(errp, errno, "failed to connect to '%s'", path);
close(sockfd);
return -1;
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/20] 9p: proxy: Fix size passed to `connect`
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 02/20] 9p: proxy: Fix size passed to `connect` Keno Fischer
@ 2018-06-01 9:09 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 9:09 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:25:57 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> The size to pass to the `connect` call is the size of the entire
> `struct sockaddr_un`. Passing anything shorter than this causes errors
> on darwin.
>
From the linux unix(7) manual page:
ret = connect (data_socket, (const struct sockaddr *) &addr,
sizeof(struct sockaddr_un));
Not sure why it was done differently, but I definitely prefer the
fixed size version.
Applied to 9p-next.
Thanks !
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
> Changes since v1: New patch
>
> hw/9pfs/9p-proxy.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index e2e0329..47a94e0 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -1088,7 +1088,7 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
>
> static int connect_namedsocket(const char *path, Error **errp)
> {
> - int sockfd, size;
> + int sockfd;
> struct sockaddr_un helper;
>
> if (strlen(path) >= sizeof(helper.sun_path)) {
> @@ -1102,8 +1102,7 @@ static int connect_namedsocket(const char *path, Error **errp)
> }
> strcpy(helper.sun_path, path);
> helper.sun_family = AF_UNIX;
> - size = strlen(helper.sun_path) + sizeof(helper.sun_family);
> - if (connect(sockfd, (struct sockaddr *)&helper, size) < 0) {
> + if (connect(sockfd, (struct sockaddr *)&helper, sizeof(helper)) < 0) {
> error_setg_errno(errp, errno, "failed to connect to '%s'", path);
> close(sockfd);
> return -1;
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 03/20] 9p: xattr: Fix crash due to free of uninitialized value
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul Keno Fischer
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 02/20] 9p: proxy: Fix size passed to `connect` Keno Fischer
@ 2018-06-01 1:25 ` Keno Fischer
2018-06-01 9:19 ` Greg Kurz
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 04/20] 9p: linux: Fix a couple Linux assumptions Keno Fischer
` (16 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
If the size returned from llistxattr is 0, we skipped the malloc
call, leaving xattr.value uninitialized. However, this value is
later passed to `g_free` without any further checks, causing an
error. Fix that by always calling g_malloc unconditionally. If
`size` is 0, it will return a pointer that is safe to pass to g_free,
likely NULL.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1: New patch
hw/9pfs/9p.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d74302d..b80db65 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3256,8 +3256,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
xattr_fidp->fs.xattr.len = size;
xattr_fidp->fid_type = P9_FID_XATTR;
xattr_fidp->fs.xattr.xattrwalk_fid = true;
+ xattr_fidp->fs.xattr.value = g_malloc0(size);
if (size) {
- xattr_fidp->fs.xattr.value = g_malloc0(size);
err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
xattr_fidp->fs.xattr.value,
xattr_fidp->fs.xattr.len);
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/20] 9p: xattr: Fix crash due to free of uninitialized value
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 03/20] 9p: xattr: Fix crash due to free of uninitialized value Keno Fischer
@ 2018-06-01 9:19 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 9:19 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:25:58 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> If the size returned from llistxattr is 0, we skipped the malloc
> call, leaving xattr.value uninitialized. However, this value is
> later passed to `g_free` without any further checks, causing an
Ouch, good catch.
> error. Fix that by always calling g_malloc unconditionally. If
> `size` is 0, it will return a pointer that is safe to pass to g_free,
> likely NULL.
>
"Allocates n_bytes bytes of memory, initialized to 0's. If n_bytes is 0 it
returns NULL."
https://developer.gnome.org/glib/unstable/glib-Memory-Allocation.html#g-malloc
The fix is good, but it seems the same can also happen if v9fs_co_lgetxattr()
returns 0 a few lines below. Can you check this out and fix it if needed ?
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
> Changes since v1: New patch
>
> hw/9pfs/9p.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index d74302d..b80db65 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3256,8 +3256,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
> xattr_fidp->fs.xattr.len = size;
> xattr_fidp->fid_type = P9_FID_XATTR;
> xattr_fidp->fs.xattr.xattrwalk_fid = true;
> + xattr_fidp->fs.xattr.value = g_malloc0(size);
> if (size) {
> - xattr_fidp->fs.xattr.value = g_malloc0(size);
> err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
> xattr_fidp->fs.xattr.value,
> xattr_fidp->fs.xattr.len);
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 04/20] 9p: linux: Fix a couple Linux assumptions
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (2 preceding siblings ...)
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 03/20] 9p: xattr: Fix crash due to free of uninitialized value Keno Fischer
@ 2018-06-01 1:25 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 05/20] 9p: Properly set errp in fstatfs error path Keno Fischer
` (15 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
- Guard Linux only headers.
- Add qemu/statfs.h header to abstract over the which
headers are needed for struct statfs
- Define `ENOATTR` only if not only defined
(it's defined in system headers on Darwin).
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1:
* New qemu/statfs.h header to factor out the header selection
to a common place. I did not write a configure check,
since the rest of 9p only supports linux/darwin anyway,
so there didn't seem to be much point, but I can write
one if requested.
* Now also covers fsdev/virtfs-proxy-helper.c
fsdev/file-op-9p.h | 2 +-
fsdev/virtfs-proxy-helper.c | 4 +++-
hw/9pfs/9p-local.c | 2 ++
include/qemu/statfs.h | 19 +++++++++++++++++++
include/qemu/xattr.h | 4 +++-
5 files changed, 28 insertions(+), 3 deletions(-)
create mode 100644 include/qemu/statfs.h
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 3fa062b..111f804 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -16,7 +16,7 @@
#include <dirent.h>
#include <utime.h>
-#include <sys/vfs.h>
+#include "qemu/statfs.h"
#include "qemu-fsdev-throttle.h"
#define SM_LOCAL_MODE_BITS 0600
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6f132c5..94fb069 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -13,17 +13,19 @@
#include <sys/resource.h>
#include <getopt.h>
#include <syslog.h>
+#ifdef CONFIG_LINUX
#include <sys/capability.h>
#include <sys/fsuid.h>
-#include <sys/vfs.h>
#include <sys/ioctl.h>
#include <linux/fs.h>
#ifdef CONFIG_LINUX_MAGIC_H
#include <linux/magic.h>
#endif
+#endif
#include "qemu-common.h"
#include "qemu/sockets.h"
#include "qemu/xattr.h"
+#include "qemu/statfs.h"
#include "9p-iov-marshal.h"
#include "hw/9pfs/9p-proxy.h"
#include "fsdev/9p-iov-marshal.h"
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index bcf2798..adc169a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -27,10 +27,12 @@
#include "qemu/error-report.h"
#include "qemu/option.h"
#include <libgen.h>
+#ifdef CONFIG_LINUX
#include <linux/fs.h>
#ifdef CONFIG_LINUX_MAGIC_H
#include <linux/magic.h>
#endif
+#endif
#include <sys/ioctl.h>
#ifndef XFS_SUPER_MAGIC
diff --git a/include/qemu/statfs.h b/include/qemu/statfs.h
new file mode 100644
index 0000000..dde289f
--- /dev/null
+++ b/include/qemu/statfs.h
@@ -0,0 +1,19 @@
+/*
+ * Host statfs header abstraction
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2, or any
+ * later version. See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef QEMU_STATFS_H
+#define QEMU_STATFS_H
+
+#ifdef CONFIG_LINUX
+# include <sys/vfs.h>
+#endif
+#ifdef CONFIG_DARWIN
+# include <sys/param.h>
+# include <sys/mount.h>
+#endif
+
+#endif
diff --git a/include/qemu/xattr.h b/include/qemu/xattr.h
index a83fe8e..f1d0f7b 100644
--- a/include/qemu/xattr.h
+++ b/include/qemu/xattr.h
@@ -22,7 +22,9 @@
#ifdef CONFIG_LIBATTR
# include <attr/xattr.h>
#else
-# define ENOATTR ENODATA
+# if !defined(ENOATTR)
+# define ENOATTR ENODATA
+# endif
# include <sys/xattr.h>
#endif
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 05/20] 9p: Properly set errp in fstatfs error path
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (3 preceding siblings ...)
2018-06-01 1:25 ` [Qemu-devel] [PATCH v2 04/20] 9p: linux: Fix a couple Linux assumptions Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 9:32 ` Greg Kurz
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined Keno Fischer
` (14 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
In the review of
9p: Avoid warning if FS_IOC_GETVERSION is not defined
Grep Kurz noted this error path was failing to set errp.
Fix that.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1: New patch
hw/9pfs/9p-local.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index adc169a..576c8e3 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1420,6 +1420,8 @@ static int local_init(FsContext *ctx, Error **errp)
*/
if (fstatfs(data->mountfd, &stbuf) < 0) {
close_preserve_errno(data->mountfd);
+ error_setg_errno(errp, errno,
+ "failed to stat file system at '%s'", ctx->fs_root);
goto err;
}
switch (stbuf.f_type) {
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/20] 9p: Properly set errp in fstatfs error path
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 05/20] 9p: Properly set errp in fstatfs error path Keno Fischer
@ 2018-06-01 9:32 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 9:32 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:26:00 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> In the review of
>
> 9p: Avoid warning if FS_IOC_GETVERSION is not defined
>
> Grep Kurz noted this error path was failing to set errp.
> Fix that.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
This is a bug fix so I've applied it to 9p-next.
Thanks!
>
> Changes since v1: New patch
>
> hw/9pfs/9p-local.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index adc169a..576c8e3 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1420,6 +1420,8 @@ static int local_init(FsContext *ctx, Error **errp)
> */
> if (fstatfs(data->mountfd, &stbuf) < 0) {
> close_preserve_errno(data->mountfd);
> + error_setg_errno(errp, errno,
> + "failed to stat file system at '%s'", ctx->fs_root);
> goto err;
> }
> switch (stbuf.f_type) {
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (4 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 05/20] 9p: Properly set errp in fstatfs error path Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 9:57 ` Greg Kurz
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 07/20] 9p: Move a couple xattr functions to 9p-util Keno Fischer
` (13 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Both `stbuf` and `local_ioc_getversion` where unused when
FS_IOC_GETVERSION was not defined, causing a compiler warning.
Reorgnaize the code to avoid this warning.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1:
* As request in review, logic is factored into a
local_ioc_getversion_init function.
hw/9pfs/9p-local.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 576c8e3..6222891 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1375,10 +1375,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
return ret;
}
+#ifdef FS_IOC_GETVERSION
static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
mode_t st_mode, uint64_t *st_gen)
{
-#ifdef FS_IOC_GETVERSION
int err;
V9fsFidOpenState fid_open;
@@ -1397,32 +1397,19 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
local_close(ctx, &fid_open);
return err;
-#else
- errno = ENOTTY;
- return -1;
-#endif
}
+#endif
-static int local_init(FsContext *ctx, Error **errp)
+static int local_ioc_getversion_init(FsContext *ctx, LocalData *data)
{
+#ifdef FS_IOC_GETVERSION
struct statfs stbuf;
- LocalData *data = g_malloc(sizeof(*data));
- data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
- if (data->mountfd == -1) {
- error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
- goto err;
- }
-
-#ifdef FS_IOC_GETVERSION
/*
* use ioc_getversion only if the ioctl is definied
*/
if (fstatfs(data->mountfd, &stbuf) < 0) {
- close_preserve_errno(data->mountfd);
- error_setg_errno(errp, errno,
- "failed to stat file system at '%s'", ctx->fs_root);
- goto err;
+ return -1;
}
switch (stbuf.f_type) {
case EXT2_SUPER_MAGIC:
@@ -1433,6 +1420,26 @@ static int local_init(FsContext *ctx, Error **errp)
break;
}
#endif
+ return 0;
+}
+
+static int local_init(FsContext *ctx, Error **errp)
+{
+ LocalData *data = g_malloc(sizeof(*data));
+
+ data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
+ if (data->mountfd == -1) {
+ error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
+ goto err;
+ }
+
+ if (local_ioc_getversion_init(ctx, data) < 0) {
+ close_preserve_errno(data->mountfd);
+ error_setg_errno(errp, errno,
+ "failed initialize ioc_getversion for file system at '%s'",
+ ctx->fs_root);
+ goto err;
+ }
if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
ctx->xops = passthrough_xattr_ops;
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined Keno Fischer
@ 2018-06-01 9:57 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 9:57 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:26:01 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> Both `stbuf` and `local_ioc_getversion` where unused when
> FS_IOC_GETVERSION was not defined, causing a compiler warning.
>
> Reorgnaize the code to avoid this warning.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
> Changes since v1:
> * As request in review, logic is factored into a
> local_ioc_getversion_init function.
>
> hw/9pfs/9p-local.c | 43 +++++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 576c8e3..6222891 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1375,10 +1375,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
> return ret;
> }
>
> +#ifdef FS_IOC_GETVERSION
> static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
> mode_t st_mode, uint64_t *st_gen)
> {
> -#ifdef FS_IOC_GETVERSION
> int err;
> V9fsFidOpenState fid_open;
>
> @@ -1397,32 +1397,19 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
> err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
> local_close(ctx, &fid_open);
> return err;
> -#else
> - errno = ENOTTY;
> - return -1;
> -#endif
> }
> +#endif
>
> -static int local_init(FsContext *ctx, Error **errp)
> +static int local_ioc_getversion_init(FsContext *ctx, LocalData *data)
> {
> +#ifdef FS_IOC_GETVERSION
> struct statfs stbuf;
> - LocalData *data = g_malloc(sizeof(*data));
>
> - data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
> - if (data->mountfd == -1) {
> - error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
> - goto err;
> - }
> -
> -#ifdef FS_IOC_GETVERSION
> /*
> * use ioc_getversion only if the ioctl is definied
> */
> if (fstatfs(data->mountfd, &stbuf) < 0) {
> - close_preserve_errno(data->mountfd);
> - error_setg_errno(errp, errno,
> - "failed to stat file system at '%s'", ctx->fs_root);
> - goto err;
Hmm, I'd prefer to keep the error_setg_errno() with fstatfs(), ie,
add an errp argument to this function.
> + return -1;
> }
> switch (stbuf.f_type) {
> case EXT2_SUPER_MAGIC:
> @@ -1433,6 +1420,26 @@ static int local_init(FsContext *ctx, Error **errp)
> break;
> }
> #endif
> + return 0;
> +}
> +
> +static int local_init(FsContext *ctx, Error **errp)
> +{
> + LocalData *data = g_malloc(sizeof(*data));
> +
> + data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
> + if (data->mountfd == -1) {
> + error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
> + goto err;
> + }
> +
> + if (local_ioc_getversion_init(ctx, data) < 0) {
> + close_preserve_errno(data->mountfd);
And this could even be a plain close()
> + error_setg_errno(errp, errno,
> + "failed initialize ioc_getversion for file system at '%s'",
True, but I think "failed to stat file system" is more meaningful,
especially with the errno.
> + ctx->fs_root);
> + goto err;
> + }
>
> if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
> ctx->xops = passthrough_xattr_ops;
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 07/20] 9p: Move a couple xattr functions to 9p-util
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (5 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 10:03 ` Greg Kurz
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 08/20] 9p: Rename 9p-util -> 9p-util-linux Keno Fischer
` (12 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
These functions will need custom implementations on Darwin. Since the
implementation is very similar among all of them, and 9p-util already
has the _nofollow version of fgetxattrat, let's move them all there.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1:
* fgetxattr_follow is dropped in favor of a different approach
later in the series.
hw/9pfs/9p-util.c | 33 +++++++++++++++++++++++++++++++++
hw/9pfs/9p-util.h | 4 ++++
hw/9pfs/9p-xattr.c | 33 ---------------------------------
3 files changed, 37 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
index f709c27..614b7fc 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util.c
@@ -24,3 +24,36 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
g_free(proc_path);
return ret;
}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+ char *list, size_t size)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = llistxattr(proc_path, list, size);
+ g_free(proc_path);
+ return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+ const char *name)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = lremovexattr(proc_path, name);
+ g_free(proc_path);
+ return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size, int flags)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = lsetxattr(proc_path, name, value, size, flags);
+ g_free(proc_path);
+ return ret;
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index dc0d2e2..79ed6b2 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -60,5 +60,9 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
void *value, size_t size);
int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
void *value, size_t size, int flags);
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+ char *list, size_t size);
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+ const char *name);
#endif
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index d05c1a1..c696d8f 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
return name_size;
}
-static ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
- char *list, size_t size)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = llistxattr(proc_path, list, size);
- g_free(proc_path);
- return ret;
-}
-
/*
* Get the list and pass to each layer to find out whether
* to send the data or not
@@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
return local_getxattr_nofollow(ctx, path, name, value, size);
}
-int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
- void *value, size_t size, int flags)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = lsetxattr(proc_path, name, value, size, flags);
- g_free(proc_path);
- return ret;
-}
-
ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
const char *name, void *value, size_t size,
int flags)
@@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
return local_setxattr_nofollow(ctx, path, name, value, size, flags);
}
-static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
- const char *name)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = lremovexattr(proc_path, name);
- g_free(proc_path);
- return ret;
-}
-
ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
const char *name)
{
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/20] 9p: Move a couple xattr functions to 9p-util
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 07/20] 9p: Move a couple xattr functions to 9p-util Keno Fischer
@ 2018-06-01 10:03 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 10:03 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:26:02 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> These functions will need custom implementations on Darwin. Since the
> implementation is very similar among all of them, and 9p-util already
> has the _nofollow version of fgetxattrat, let's move them all there.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
This cleanup makes sense irrespective of the rest of the series.
Applied to 9p-next.
Thanks!
> Changes since v1:
> * fgetxattr_follow is dropped in favor of a different approach
> later in the series.
>
> hw/9pfs/9p-util.c | 33 +++++++++++++++++++++++++++++++++
> hw/9pfs/9p-util.h | 4 ++++
> hw/9pfs/9p-xattr.c | 33 ---------------------------------
> 3 files changed, 37 insertions(+), 33 deletions(-)
>
> diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> index f709c27..614b7fc 100644
> --- a/hw/9pfs/9p-util.c
> +++ b/hw/9pfs/9p-util.c
> @@ -24,3 +24,36 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> g_free(proc_path);
> return ret;
> }
> +
> +ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> + char *list, size_t size)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = llistxattr(proc_path, list, size);
> + g_free(proc_path);
> + return ret;
> +}
> +
> +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> + const char *name)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = lremovexattr(proc_path, name);
> + g_free(proc_path);
> + return ret;
> +}
> +
> +int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> + void *value, size_t size, int flags)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = lsetxattr(proc_path, name, value, size, flags);
> + g_free(proc_path);
> + return ret;
> +}
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index dc0d2e2..79ed6b2 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -60,5 +60,9 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
> void *value, size_t size);
> int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
> void *value, size_t size, int flags);
> +ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> + char *list, size_t size);
> +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> + const char *name);
>
> #endif
> diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
> index d05c1a1..c696d8f 100644
> --- a/hw/9pfs/9p-xattr.c
> +++ b/hw/9pfs/9p-xattr.c
> @@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
> return name_size;
> }
>
> -static ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> - char *list, size_t size)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = llistxattr(proc_path, list, size);
> - g_free(proc_path);
> - return ret;
> -}
> -
> /*
> * Get the list and pass to each layer to find out whether
> * to send the data or not
> @@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
> return local_getxattr_nofollow(ctx, path, name, value, size);
> }
>
> -int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> - void *value, size_t size, int flags)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = lsetxattr(proc_path, name, value, size, flags);
> - g_free(proc_path);
> - return ret;
> -}
> -
> ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
> const char *name, void *value, size_t size,
> int flags)
> @@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
> return local_setxattr_nofollow(ctx, path, name, value, size, flags);
> }
>
> -static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> - const char *name)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = lremovexattr(proc_path, name);
> - g_free(proc_path);
> - return ret;
> -}
> -
> ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
> const char *name)
> {
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 08/20] 9p: Rename 9p-util -> 9p-util-linux
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (6 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 07/20] 9p: Move a couple xattr functions to 9p-util Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 10:07 ` Greg Kurz
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 09/20] 9p: Properly check/translate flags in unlinkat Keno Fischer
` (11 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
The current file only has the Linux versions of these functions.
Rename the file accordingly and update the Makefile to only build
it on Linux. A Darwin version of these will follow later in the
series.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1: New patch
hw/9pfs/9p-util-linux.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/9pfs/9p-util.c | 59 -------------------------------------------------
hw/9pfs/Makefile.objs | 3 ++-
3 files changed, 61 insertions(+), 60 deletions(-)
create mode 100644 hw/9pfs/9p-util-linux.c
delete mode 100644 hw/9pfs/9p-util.c
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
new file mode 100644
index 0000000..defa3a4
--- /dev/null
+++ b/hw/9pfs/9p-util-linux.c
@@ -0,0 +1,59 @@
+/*
+ * 9p utilities (Linux Implementation)
+ *
+ * Copyright IBM, Corp. 2017
+ *
+ * Authors:
+ * Greg Kurz <groug@kaod.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = lgetxattr(proc_path, name, value, size);
+ g_free(proc_path);
+ return ret;
+}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+ char *list, size_t size)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = llistxattr(proc_path, list, size);
+ g_free(proc_path);
+ return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+ const char *name)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = lremovexattr(proc_path, name);
+ g_free(proc_path);
+ return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size, int flags)
+{
+ char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+ int ret;
+
+ ret = lsetxattr(proc_path, name, value, size, flags);
+ g_free(proc_path);
+ return ret;
+}
diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
deleted file mode 100644
index 614b7fc..0000000
--- a/hw/9pfs/9p-util.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- * 9p utilities
- *
- * Copyright IBM, Corp. 2017
- *
- * Authors:
- * Greg Kurz <groug@kaod.org>
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-#include "qemu/xattr.h"
-#include "9p-util.h"
-
-ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
- void *value, size_t size)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = lgetxattr(proc_path, name, value, size);
- g_free(proc_path);
- return ret;
-}
-
-ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
- char *list, size_t size)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = llistxattr(proc_path, list, size);
- g_free(proc_path);
- return ret;
-}
-
-ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
- const char *name)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = lremovexattr(proc_path, name);
- g_free(proc_path);
- return ret;
-}
-
-int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
- void *value, size_t size, int flags)
-{
- char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
- int ret;
-
- ret = lsetxattr(proc_path, name, value, size, flags);
- g_free(proc_path);
- return ret;
-}
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index fd90b62..083508f 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,4 +1,5 @@
-common-obj-y = 9p.o 9p-util.o
+common-obj-y = 9p.o
+common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
common-obj-y += 9p-local.o 9p-xattr.o
common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
common-obj-y += coth.o cofs.o codir.o cofile.o
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/20] 9p: Rename 9p-util -> 9p-util-linux
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 08/20] 9p: Rename 9p-util -> 9p-util-linux Keno Fischer
@ 2018-06-01 10:07 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 10:07 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:26:03 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> The current file only has the Linux versions of these functions.
> Rename the file accordingly and update the Makefile to only build
> it on Linux. A Darwin version of these will follow later in the
> series.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
Reviewed-by: Greg Kurz <groug@kaod.org>
> Changes since v1: New patch
>
> hw/9pfs/9p-util-linux.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
> hw/9pfs/9p-util.c | 59 -------------------------------------------------
> hw/9pfs/Makefile.objs | 3 ++-
> 3 files changed, 61 insertions(+), 60 deletions(-)
> create mode 100644 hw/9pfs/9p-util-linux.c
> delete mode 100644 hw/9pfs/9p-util.c
>
> diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
> new file mode 100644
> index 0000000..defa3a4
> --- /dev/null
> +++ b/hw/9pfs/9p-util-linux.c
> @@ -0,0 +1,59 @@
> +/*
> + * 9p utilities (Linux Implementation)
> + *
> + * Copyright IBM, Corp. 2017
> + *
> + * Authors:
> + * Greg Kurz <groug@kaod.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/xattr.h"
> +#include "9p-util.h"
> +
> +ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> + void *value, size_t size)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = lgetxattr(proc_path, name, value, size);
> + g_free(proc_path);
> + return ret;
> +}
> +
> +ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> + char *list, size_t size)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = llistxattr(proc_path, list, size);
> + g_free(proc_path);
> + return ret;
> +}
> +
> +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> + const char *name)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = lremovexattr(proc_path, name);
> + g_free(proc_path);
> + return ret;
> +}
> +
> +int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> + void *value, size_t size, int flags)
> +{
> + char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> + int ret;
> +
> + ret = lsetxattr(proc_path, name, value, size, flags);
> + g_free(proc_path);
> + return ret;
> +}
> diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> deleted file mode 100644
> index 614b7fc..0000000
> --- a/hw/9pfs/9p-util.c
> +++ /dev/null
> @@ -1,59 +0,0 @@
> -/*
> - * 9p utilities
> - *
> - * Copyright IBM, Corp. 2017
> - *
> - * Authors:
> - * Greg Kurz <groug@kaod.org>
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2 or later.
> - * See the COPYING file in the top-level directory.
> - */
> -
> -#include "qemu/osdep.h"
> -#include "qemu/xattr.h"
> -#include "9p-util.h"
> -
> -ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> - void *value, size_t size)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = lgetxattr(proc_path, name, value, size);
> - g_free(proc_path);
> - return ret;
> -}
> -
> -ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> - char *list, size_t size)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = llistxattr(proc_path, list, size);
> - g_free(proc_path);
> - return ret;
> -}
> -
> -ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> - const char *name)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = lremovexattr(proc_path, name);
> - g_free(proc_path);
> - return ret;
> -}
> -
> -int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> - void *value, size_t size, int flags)
> -{
> - char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
> - int ret;
> -
> - ret = lsetxattr(proc_path, name, value, size, flags);
> - g_free(proc_path);
> - return ret;
> -}
> diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
> index fd90b62..083508f 100644
> --- a/hw/9pfs/Makefile.objs
> +++ b/hw/9pfs/Makefile.objs
> @@ -1,4 +1,5 @@
> -common-obj-y = 9p.o 9p-util.o
> +common-obj-y = 9p.o
> +common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
> common-obj-y += 9p-local.o 9p-xattr.o
> common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
> common-obj-y += coth.o cofs.o codir.o cofile.o
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 09/20] 9p: Properly check/translate flags in unlinkat
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (7 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 08/20] 9p: Rename 9p-util -> 9p-util-linux Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 10:13 ` Greg Kurz
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 10/20] 9p: darwin: Handle struct stat(fs) differences Keno Fischer
` (10 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
This code previously relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR
having the same numerical value and deferred any errorchecking to the
syscall itself. However, while the former assumption is true on Linux,
it is not true in general. Thus, add appropriate error checking and
translation to the 9p unlinkat server code.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1:
* Code was moved from 9p-local.c to server entry point in 9p.c
hw/9pfs/9p.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b80db65..a757374 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2522,7 +2522,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
{
int err = 0;
V9fsString name;
- int32_t dfid, flags;
+ int32_t dfid, flags, rflags = 0;
size_t offset = 7;
V9fsPath path;
V9fsFidState *dfidp;
@@ -2549,6 +2549,15 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
goto out_nofid;
}
+ if (flags & ~P9_DOTL_AT_REMOVEDIR) {
+ err = -EINVAL;
+ goto out_nofid;
+ }
+
+ if (flags & P9_DOTL_AT_REMOVEDIR) {
+ rflags |= AT_REMOVEDIR;
+ }
+
dfidp = get_fid(pdu, dfid);
if (dfidp == NULL) {
err = -EINVAL;
@@ -2567,7 +2576,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
if (err < 0) {
goto out_err;
}
- err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, flags);
+ err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, rflags);
if (!err) {
err = offset;
}
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/20] 9p: Properly check/translate flags in unlinkat
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 09/20] 9p: Properly check/translate flags in unlinkat Keno Fischer
@ 2018-06-01 10:13 ` Greg Kurz
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 10:13 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:26:04 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> This code previously relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR
> having the same numerical value and deferred any errorchecking to the
> syscall itself. However, while the former assumption is true on Linux,
> it is not true in general. Thus, add appropriate error checking and
> translation to the 9p unlinkat server code.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
Looks good but handle_unlinkat() needs to be adapted to this change.
Other backends (proxy and synth) seem to ignore the flags.
> Changes since v1:
> * Code was moved from 9p-local.c to server entry point in 9p.c
>
> hw/9pfs/9p.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index b80db65..a757374 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -2522,7 +2522,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
> {
> int err = 0;
> V9fsString name;
> - int32_t dfid, flags;
> + int32_t dfid, flags, rflags = 0;
> size_t offset = 7;
> V9fsPath path;
> V9fsFidState *dfidp;
> @@ -2549,6 +2549,15 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
> goto out_nofid;
> }
>
> + if (flags & ~P9_DOTL_AT_REMOVEDIR) {
> + err = -EINVAL;
> + goto out_nofid;
> + }
> +
> + if (flags & P9_DOTL_AT_REMOVEDIR) {
> + rflags |= AT_REMOVEDIR;
> + }
> +
> dfidp = get_fid(pdu, dfid);
> if (dfidp == NULL) {
> err = -EINVAL;
> @@ -2567,7 +2576,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
> if (err < 0) {
> goto out_err;
> }
> - err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, flags);
> + err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, rflags);
> if (!err) {
> err = offset;
> }
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 10/20] 9p: darwin: Handle struct stat(fs) differences
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (8 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 09/20] 9p: Properly check/translate flags in unlinkat Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 11/20] 9p: darwin: Handle struct dirent differences Keno Fischer
` (9 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1:
* Now also covers fsdev/virtfs-proxy-helper.c
fsdev/virtfs-proxy-helper.c | 14 +++++++++++---
hw/9pfs/9p-proxy.c | 17 ++++++++++++++---
hw/9pfs/9p-synth.c | 2 ++
hw/9pfs/9p.c | 16 ++++++++++++++--
4 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 94fb069..3bc1269 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -506,12 +506,15 @@ static void stat_to_prstat(ProxyStat *pr_stat, struct stat *stat)
pr_stat->st_size = stat->st_size;
pr_stat->st_blksize = stat->st_blksize;
pr_stat->st_blocks = stat->st_blocks;
+#ifdef CONFIG_DARWIN
+ pr_stat->st_atim_nsec = stat->st_atimespec.tv_nsec;
+ pr_stat->st_mtim_nsec = stat->st_mtimespec.tv_nsec;
+ pr_stat->st_ctim_nsec = stat->st_ctimespec.tv_nsec;
+#else
pr_stat->st_atim_sec = stat->st_atim.tv_sec;
- pr_stat->st_atim_nsec = stat->st_atim.tv_nsec;
pr_stat->st_mtim_sec = stat->st_mtim.tv_sec;
- pr_stat->st_mtim_nsec = stat->st_mtim.tv_nsec;
pr_stat->st_ctim_sec = stat->st_ctim.tv_sec;
- pr_stat->st_ctim_nsec = stat->st_ctim.tv_nsec;
+#endif
}
static void statfs_to_prstatfs(ProxyStatFS *pr_stfs, struct statfs *stfs)
@@ -524,10 +527,15 @@ static void statfs_to_prstatfs(ProxyStatFS *pr_stfs, struct statfs *stfs)
pr_stfs->f_bavail = stfs->f_bavail;
pr_stfs->f_files = stfs->f_files;
pr_stfs->f_ffree = stfs->f_ffree;
+#ifdef CONFIG_DARWIN
+ pr_stfs->f_fsid[0] = stfs->f_fsid.val[0];
+ pr_stfs->f_fsid[1] = stfs->f_fsid.val[1];
+#else
pr_stfs->f_fsid[0] = stfs->f_fsid.__val[0];
pr_stfs->f_fsid[1] = stfs->f_fsid.__val[1];
pr_stfs->f_namelen = stfs->f_namelen;
pr_stfs->f_frsize = stfs->f_frsize;
+#endif
}
/*
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 47a94e0..8a2c174 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -117,10 +117,15 @@ static void prstatfs_to_statfs(struct statfs *stfs, ProxyStatFS *prstfs)
stfs->f_bavail = prstfs->f_bavail;
stfs->f_files = prstfs->f_files;
stfs->f_ffree = prstfs->f_ffree;
+#ifdef CONFIG_DARWIN
+ stfs->f_fsid.val[0] = prstfs->f_fsid[0] & 0xFFFFFFFFU;
+ stfs->f_fsid.val[1] = prstfs->f_fsid[1] >> 32 & 0xFFFFFFFFU;
+#else
stfs->f_fsid.__val[0] = prstfs->f_fsid[0] & 0xFFFFFFFFU;
stfs->f_fsid.__val[1] = prstfs->f_fsid[1] >> 32 & 0xFFFFFFFFU;
stfs->f_namelen = prstfs->f_namelen;
stfs->f_frsize = prstfs->f_frsize;
+#endif
}
/* Converts proxy_stat structure to VFS stat structure */
@@ -137,12 +142,18 @@ static void prstat_to_stat(struct stat *stbuf, ProxyStat *prstat)
stbuf->st_size = prstat->st_size;
stbuf->st_blksize = prstat->st_blksize;
stbuf->st_blocks = prstat->st_blocks;
- stbuf->st_atim.tv_sec = prstat->st_atim_sec;
- stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
+ stbuf->st_atime = prstat->st_atim_sec;
stbuf->st_mtime = prstat->st_mtim_sec;
- stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
stbuf->st_ctime = prstat->st_ctim_sec;
+#ifdef CONFIG_DARWIN
+ stbuf->st_atimespec.tv_nsec = prstat->st_atim_nsec;
+ stbuf->st_mtimespec.tv_nsec = prstat->st_mtim_nsec;
+ stbuf->st_ctimespec.tv_nsec = prstat->st_ctim_nsec;
+#else
+ stbuf->st_atim.tv_nsec = prstat->st_atim_nsec;
+ stbuf->st_mtim.tv_nsec = prstat->st_mtim_nsec;
stbuf->st_ctim.tv_nsec = prstat->st_ctim_nsec;
+#endif
}
/*
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 54239c9..eb68b42 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -426,7 +426,9 @@ static int synth_statfs(FsContext *s, V9fsPath *fs_path,
stbuf->f_bsize = 512;
stbuf->f_blocks = 0;
stbuf->f_files = synth_node_count;
+#ifndef CONFIG_DARWIN
stbuf->f_namelen = NAME_MAX;
+#endif
return 0;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index a757374..212f569 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -905,11 +905,17 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
v9lstat->st_blksize = stbuf->st_blksize;
v9lstat->st_blocks = stbuf->st_blocks;
v9lstat->st_atime_sec = stbuf->st_atime;
- v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
v9lstat->st_mtime_sec = stbuf->st_mtime;
- v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
v9lstat->st_ctime_sec = stbuf->st_ctime;
+#ifdef CONFIG_DARWIN
+ v9lstat->st_atime_nsec = stbuf->st_atimespec.tv_nsec;
+ v9lstat->st_mtime_nsec = stbuf->st_mtimespec.tv_nsec;
+ v9lstat->st_ctime_nsec = stbuf->st_ctimespec.tv_nsec;
+#else
+ v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
+ v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec;
+#endif
/* Currently we only support BASIC fields in stat */
v9lstat->st_result_mask = P9_STATS_BASIC;
@@ -2959,9 +2965,15 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU *pdu, struct statfs *stbuf)
f_bavail = stbuf->f_bavail/bsize_factor;
f_files = stbuf->f_files;
f_ffree = stbuf->f_ffree;
+#ifdef CONFIG_DARWIN
+ fsid_val = (unsigned int)stbuf->f_fsid.val[0] |
+ (unsigned long long)stbuf->f_fsid.val[1] << 32;
+ f_namelen = MAXNAMLEN;
+#else
fsid_val = (unsigned int) stbuf->f_fsid.__val[0] |
(unsigned long long)stbuf->f_fsid.__val[1] << 32;
f_namelen = stbuf->f_namelen;
+#endif
return pdu_marshal(pdu, offset, "ddqqqqqqd",
f_type, f_bsize, f_blocks, f_bfree,
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 11/20] 9p: darwin: Handle struct dirent differences
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (9 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 10/20] 9p: darwin: Handle struct stat(fs) differences Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1 Keno Fischer
` (8 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
On darwin d_seekoff exists, but is optional and does not seem to
be commonly used by file systems. Use `telldir` instead to obtain
the seek offset.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes since v1:
* Drop setting d_seekoff in synth_direntry
* Factor telldir vs d_off logic into v9fs_dent_telldir
* Error path for telldir failure
hw/9pfs/9p-synth.c | 2 ++
hw/9pfs/9p.c | 36 ++++++++++++++++++++++++++++++++----
2 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index eb68b42..a312f8c 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -221,7 +221,9 @@ static void synth_direntry(V9fsSynthNode *node,
{
strcpy(entry->d_name, node->name);
entry->d_ino = node->attr->inode;
+#ifndef CONFIG_DARWIN
entry->d_off = off + 1;
+#endif
}
static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 212f569..9751246 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1738,6 +1738,25 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
return offset;
}
+/**
+ * Get the seek offset of a dirent. If not available from the structure itself,
+ * obtain it by calling telldir.
+ */
+static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct dirent *dent)
+{
+#ifdef CONFIG_DARWIN
+ /*
+ * Darwin has d_seekoff, which appears to function similarly to d_off.
+ * However, it does not appear to be supported on all file systems,
+ * so use telldir for correctness.
+ */
+ return v9fs_co_telldir(pdu, fidp);
+#else
+ return dent->d_off;
+#endif
+}
+
static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
V9fsFidState *fidp,
uint32_t max_count)
@@ -1801,7 +1820,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
count += len;
v9fs_stat_free(&v9stat);
v9fs_path_free(&path);
- saved_dir_pos = dent->d_off;
+ saved_dir_pos = v9fs_dent_telldir(pdu, fidp, dent);
+ if (saved_dir_pos < 0) {
+ err = saved_dir_pos;
+ break;
+ }
}
v9fs_readdir_unlock(&fidp->fs.dir);
@@ -1915,7 +1938,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
V9fsString name;
int len, err = 0;
int32_t count = 0;
- off_t saved_dir_pos;
+ off_t saved_dir_pos, off;
struct dirent *dent;
/* save the directory position */
@@ -1951,10 +1974,15 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
/* Fill the other fields with dummy values */
qid.type = 0;
qid.version = 0;
+ off = v9fs_dent_telldir(pdu, fidp, dent);
+ if (off < 0) {
+ err = off;
+ break;
+ }
/* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
len = pdu_marshal(pdu, 11 + count, "Qqbs",
- &qid, dent->d_off,
+ &qid, off,
dent->d_type, &name);
v9fs_readdir_unlock(&fidp->fs.dir);
@@ -1966,7 +1994,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
}
count += len;
v9fs_string_free(&name);
- saved_dir_pos = dent->d_off;
+ saved_dir_pos = off;
}
v9fs_readdir_unlock(&fidp->fs.dir);
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (10 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 11/20] 9p: darwin: Handle struct dirent differences Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-29 20:32 ` Eric Blake
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 13/20] 9p: darwin: Ignore O_{NOATIME, DIRECT} Keno Fischer
` (7 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Comparisons of mode_t with -1 require an explicit cast, since mode_t
is unsigned on Darwin.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1: Split from strchrnul change
hw/9pfs/9p-local.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 6222891..1f0b1b0 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -310,7 +310,7 @@ update_map_file:
if (credp->fc_gid != -1) {
gid = credp->fc_gid;
}
- if (credp->fc_mode != -1) {
+ if (credp->fc_mode != (mode_t)-1) {
mode = credp->fc_mode;
}
if (credp->fc_rdev != -1) {
@@ -416,7 +416,7 @@ static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
return err;
}
}
- if (credp->fc_mode != -1) {
+ if (credp->fc_mode != (mode_t)-1) {
uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode,
sizeof(mode_t), 0);
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1 Keno Fischer
@ 2018-06-29 20:32 ` Eric Blake
0 siblings, 0 replies; 34+ messages in thread
From: Eric Blake @ 2018-06-29 20:32 UTC (permalink / raw)
To: Keno Fischer, qemu-devel; +Cc: groug
On 05/31/2018 08:26 PM, Keno Fischer wrote:
> Comparisons of mode_t with -1 require an explicit cast, since mode_t
> is unsigned on Darwin.
It's not JUST that mode_t is unsigned (an unsigned int compares just
fine against -1), but ALSO that mode_t has unspecified width. That is,
you cannot portably assume whether mode_t is smaller, equivalent, or
larger rank than int. If it is smaller, then you can't use mode_t in
va_arg(), and mode_t will promote to signed int, whether or not mode_t
is unsigned; but '((mode_t)-1) == -1' is going to be false if mode_t is
unsigned (because the cast truncates the sign extension bits into a
positive value). Conversely, since mode_t can be larger than int
(although I know of no such platform that does so), blindly using 'int'
when trying to parse a mode_t argument through va_arg() will truncate bits.
So, for example, to portably write a wrapper around open(), you HAVE to
use hairy constructions like:
mode = (sizeof (mode_t) < sizeof (int)
? va_arg (ap, int)
: va_arg (ap, mode_t));
or, to avoid spurious compiler warnings on the branch not taken, define
a macro learned at configure time. This is what gnulib does, for example:
AC_CACHE_CHECK([for promoted mode_t type], [gl_cv_promoted_mode_t], [
dnl Assume mode_t promotes to 'int' if and only if it is smaller
than 'int',
dnl and to itself otherwise. This assumption is not guaranteed by
the ISO C
dnl standard, but we don't know of any real-world counterexamples.
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <sys/types.h>]],
[[typedef int array[2 * (sizeof (mode_t) < sizeof (int)) - 1];]])],
[gl_cv_promoted_mode_t='int'],
[gl_cv_promoted_mode_t='mode_t'])
])
AC_DEFINE_UNQUOTED([PROMOTED_MODE_T], [$gl_cv_promoted_mode_t],
[Define to the type that is the result of default argument
promotions of type mode_t.])
> +++ b/hw/9pfs/9p-local.c
> @@ -310,7 +310,7 @@ update_map_file:
> if (credp->fc_gid != -1) {
> gid = credp->fc_gid;
> }
> - if (credp->fc_mode != -1) {
> + if (credp->fc_mode != (mode_t)-1) {
At any rate, this is the correct portability fix for this code.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 13/20] 9p: darwin: Ignore O_{NOATIME, DIRECT}
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (11 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1 Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 14/20] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX Keno Fischer
` (6 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Darwin doesn't have either of these flags. Darwin does have
F_NOCACHE, which is similar to O_DIRECT, but has different
enough semantics that other projects don't generally map
them automatically. In any case, we don't support O_DIRECT
on Linux at the moment either.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1: Undo accidental formatting change
hw/9pfs/9p.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 9751246..70cfab9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -123,11 +123,18 @@ static int dotl_to_open_flags(int flags)
{ P9_DOTL_NONBLOCK, O_NONBLOCK } ,
{ P9_DOTL_DSYNC, O_DSYNC },
{ P9_DOTL_FASYNC, FASYNC },
+#ifndef CONFIG_DARWIN
+ { P9_DOTL_NOATIME, O_NOATIME },
+ /* On Darwin, we could map to F_NOCACHE, which is
+ similar, but doesn't quite have the same
+ semantics. However, we don't support O_DIRECT
+ even on linux at the moment, so we just ignore
+ it here. */
{ P9_DOTL_DIRECT, O_DIRECT },
+#endif
{ P9_DOTL_LARGEFILE, O_LARGEFILE },
{ P9_DOTL_DIRECTORY, O_DIRECTORY },
{ P9_DOTL_NOFOLLOW, O_NOFOLLOW },
- { P9_DOTL_NOATIME, O_NOATIME },
{ P9_DOTL_SYNC, O_SYNC },
};
@@ -156,10 +163,12 @@ static int get_dotl_openflags(V9fsState *s, int oflags)
*/
flags = dotl_to_open_flags(oflags);
flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
+#ifndef CONFIG_DARWIN
/*
* Ignore direct disk access hint until the server supports it.
*/
flags &= ~O_DIRECT;
+#endif
return flags;
}
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 14/20] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (12 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 13/20] 9p: darwin: Ignore O_{NOATIME, DIRECT} Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations Keno Fischer
` (5 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
No change from v1.
hw/9pfs/9p.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 70cfab9..24802b9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3374,6 +3374,13 @@ out_nofid:
v9fs_string_free(&name);
}
+#if defined(CONFIG_DARWIN) && !defined(XATTR_SIZE_MAX)
+/* Darwin doesn't seem to define a maximum xattr size in its user
+ user space header, but looking at the kernel source, HFS supports
+ up to INT32_MAX, so use that as the maximum.
+*/
+#define XATTR_SIZE_MAX INT32_MAX
+#endif
static void coroutine_fn v9fs_xattrcreate(void *opaque)
{
int flags;
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (13 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 14/20] 9p: darwin: Provide a compatibility definition for XATTR_SIZE_MAX Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 11:13 ` Greg Kurz
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 16/20] 9p: darwin: Compatibility for f/l*xattr Keno Fischer
` (4 subsequent siblings)
19 siblings, 1 reply; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
This implements the darwin equivalent of the functions that were
moved to 9p-util(-linux) earlier in this series in the new
9p-util-darwin file.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1:
* New 9p-util-darwin.c rather than ifdefs in 9p-util.c
* Drop incorrect AT_NOFOLLOW from the actual call
hw/9pfs/9p-util-darwin.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
hw/9pfs/Makefile.objs | 1 +
2 files changed, 65 insertions(+)
create mode 100644 hw/9pfs/9p-util-darwin.c
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
new file mode 100644
index 0000000..cdb4c9e
--- /dev/null
+++ b/hw/9pfs/9p-util-darwin.c
@@ -0,0 +1,64 @@
+/*
+ * 9p utilities (Darwin Implementation)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xattr.h"
+#include "9p-util.h"
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size)
+{
+ int ret;
+ int fd = openat_file(dirfd, filename,
+ O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+ if (fd == -1) {
+ return -1;
+ }
+ ret = fgetxattr(fd, name, value, size, 0, 0);
+ close_preserve_errno(fd);
+ return ret;
+}
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+ char *list, size_t size)
+{
+ int ret;
+ int fd = openat_file(dirfd, filename,
+ O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+ if (fd == -1) {
+ return -1;
+ }
+ ret = flistxattr(fd, list, size, 0);
+ close_preserve_errno(fd);
+ return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+ const char *name)
+{
+ int ret;
+ int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+ if (fd == -1) {
+ return -1;
+ }
+ ret = fremovexattr(fd, name, 0);
+ close_preserve_errno(fd);
+ return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+ void *value, size_t size, int flags)
+{
+ int ret;
+ int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+ if (fd == -1) {
+ return -1;
+ }
+ ret = fsetxattr(fd, name, value, size, 0, flags);
+ close_preserve_errno(fd);
+ return ret;
+}
diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
index 083508f..24a8695 100644
--- a/hw/9pfs/Makefile.objs
+++ b/hw/9pfs/Makefile.objs
@@ -1,5 +1,6 @@
common-obj-y = 9p.o
common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
+common-obj-$(CONFIG_DARWIN) += 9p-util-darwin.o
common-obj-y += 9p-local.o 9p-xattr.o
common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
common-obj-y += coth.o cofs.o codir.o cofile.o
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations Keno Fischer
@ 2018-06-01 11:13 ` Greg Kurz
2018-06-02 20:01 ` Keno Fischer
0 siblings, 1 reply; 34+ messages in thread
From: Greg Kurz @ 2018-06-01 11:13 UTC (permalink / raw)
To: Keno Fischer; +Cc: qemu-devel
On Thu, 31 May 2018 21:26:10 -0400
Keno Fischer <keno@juliacomputing.com> wrote:
> This implements the darwin equivalent of the functions that were
> moved to 9p-util(-linux) earlier in this series in the new
> 9p-util-darwin file.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>
The patch looks good but...
> Changes from v1:
> * New 9p-util-darwin.c rather than ifdefs in 9p-util.c
> * Drop incorrect AT_NOFOLLOW from the actual call
>
> hw/9pfs/9p-util-darwin.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
> hw/9pfs/Makefile.objs | 1 +
> 2 files changed, 65 insertions(+)
> create mode 100644 hw/9pfs/9p-util-darwin.c
>
> diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
> new file mode 100644
> index 0000000..cdb4c9e
> --- /dev/null
> +++ b/hw/9pfs/9p-util-darwin.c
> @@ -0,0 +1,64 @@
> +/*
> + * 9p utilities (Darwin Implementation)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/xattr.h"
> +#include "9p-util.h"
> +
> +ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> + void *value, size_t size)
> +{
> + int ret;
> + int fd = openat_file(dirfd, filename,
> + O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> + if (fd == -1) {
> + return -1;
> + }
> + ret = fgetxattr(fd, name, value, size, 0, 0);
> + close_preserve_errno(fd);
> + return ret;
> +}
> +
> +ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
> + char *list, size_t size)
> +{
> + int ret;
> + int fd = openat_file(dirfd, filename,
> + O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> + if (fd == -1) {
> + return -1;
> + }
> + ret = flistxattr(fd, list, size, 0);
> + close_preserve_errno(fd);
> + return ret;
> +}
> +
> +ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
> + const char *name)
> +{
> + int ret;
> + int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> + if (fd == -1) {
> + return -1;
> + }
> + ret = fremovexattr(fd, name, 0);
> + close_preserve_errno(fd);
> + return ret;
> +}
> +
> +int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
> + void *value, size_t size, int flags)
> +{
> + int ret;
> + int fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
> + if (fd == -1) {
> + return -1;
> + }
> + ret = fsetxattr(fd, name, value, size, 0, flags);
> + close_preserve_errno(fd);
> + return ret;
> +}
... I now realize that flags may come from the client, ie, it should be
translated before being passed to the backends, pretty much like the other
patch with unlinkat.
The specification for 9p2000.L says it is derived from "Linux setxattr".
https://github.com/chaos/diod/blob/master/protocol.md#xattrcreate----prepare-to-set-extended-attribute
ie,
include/uapi/linux/xattr.h:#define XATTR_CREATE 0x1 /* set value, fail if attr already exists */
include/uapi/linux/xattr.h:#define XATTR_REPLACE 0x2 /* set value, fail if attr does not exist */
I guess this calls for some defines in 9p.h:
/* 9p2000.L says that the 'flags' argument of operation 'xattrcreate'
* are derived from Linux setxattr.
*/
#define P9_XATTR_CREATE 1
#define P9_XATTR_REPLACE 2
Please do that in a preparatory patch.
I would also appreciate you look at other 9P operations and
check if we have other places where we need to translate
some flags.
> diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
> index 083508f..24a8695 100644
> --- a/hw/9pfs/Makefile.objs
> +++ b/hw/9pfs/Makefile.objs
> @@ -1,5 +1,6 @@
> common-obj-y = 9p.o
> common-obj-$(CONFIG_LINUX) += 9p-util-linux.o
> +common-obj-$(CONFIG_DARWIN) += 9p-util-darwin.o
> common-obj-y += 9p-local.o 9p-xattr.o
> common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
> common-obj-y += coth.o cofs.o codir.o cofile.o
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations
2018-06-01 11:13 ` Greg Kurz
@ 2018-06-02 20:01 ` Keno Fischer
0 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-02 20:01 UTC (permalink / raw)
To: Greg Kurz; +Cc: QEMU Developers
> I guess this calls for some defines in 9p.h:
>
> /* 9p2000.L says that the 'flags' argument of operation 'xattrcreate'
> * are derived from Linux setxattr.
> */
> #define P9_XATTR_CREATE 1
> #define P9_XATTR_REPLACE 2
>
> Please do that in a preparatory patch.
>
> I would also appreciate you look at other 9P operations and
> check if we have other places where we need to translate
> some flags.
I will include this additional patch in the next respin of the series.
I took a look at the remaining protocol messages and it looks
like with the exception of this and unlinkat (the other patch in the
series), flags/open modes are translated properly.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v2 16/20] 9p: darwin: Compatibility for f/l*xattr
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (14 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 15/20] 9p: darwin: *xattr_nofollow implementations Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 17/20] 9p: darwin: Provide a fallback implementation for utimensat Keno Fischer
` (3 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
On darwin `fgetxattr` takes two extra optional arguments,
and the l* variants are not defined (in favor of an extra
flag to the regular variants.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1: New patch, qemu_fgetxattr had previously been
moved to 9p-util as fgetxattr_follow. The remaining functions
are required by the proxy-helper.
Makefile | 6 ++++++
fsdev/virtfs-proxy-helper.c | 9 +++++----
hw/9pfs/9p-local.c | 12 ++++++++----
hw/9pfs/9p-util.h | 17 +++++++++++++++++
4 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/Makefile b/Makefile
index 6d588d1..dac6efd 100644
--- a/Makefile
+++ b/Makefile
@@ -544,7 +544,13 @@ qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o $(COMMON_LDADDS)
qemu-keymap$(EXESUF): qemu-keymap.o ui/input-keymap.o $(COMMON_LDADDS)
fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o fsdev/9p-marshal.o fsdev/9p-iov-marshal.o $(COMMON_LDADDS)
+ifdef CONFIG_DARWIN
+fsdev/virtfs-proxy-helper$(EXESUF): hw/9pfs/9p-util-darwin.o
+endif
+ifdef CONFIG_LINUX
+fsdev/virtfs-proxy-helper$(EXESUF): hw/9pfs/9p-util-linux.o
fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
+endif
scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
ifdef CONFIG_MPATH
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 3bc1269..a26f8b8 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -28,6 +28,7 @@
#include "qemu/statfs.h"
#include "9p-iov-marshal.h"
#include "hw/9pfs/9p-proxy.h"
+#include "hw/9pfs/9p-util.h"
#include "fsdev/9p-iov-marshal.h"
#define PROGNAME "virtfs-proxy-helper"
@@ -459,7 +460,7 @@ static int do_getxattr(int type, struct iovec *iovec, struct iovec *out_iovec)
v9fs_string_init(&name);
retval = proxy_unmarshal(iovec, offset, "s", &name);
if (retval > 0) {
- retval = lgetxattr(path.data, name.data, xattr.data, size);
+ retval = qemu_lgetxattr(path.data, name.data, xattr.data, size);
if (retval < 0) {
retval = -errno;
} else {
@@ -469,7 +470,7 @@ static int do_getxattr(int type, struct iovec *iovec, struct iovec *out_iovec)
v9fs_string_free(&name);
break;
case T_LLISTXATTR:
- retval = llistxattr(path.data, xattr.data, size);
+ retval = qemu_llistxattr(path.data, xattr.data, size);
if (retval < 0) {
retval = -errno;
} else {
@@ -1000,7 +1001,7 @@ static int process_requests(int sock)
retval = proxy_unmarshal(&in_iovec, PROXY_HDR_SZ, "sssdd", &path,
&name, &value, &size, &flags);
if (retval > 0) {
- retval = lsetxattr(path.data,
+ retval = qemu_lsetxattr(path.data,
name.data, value.data, size, flags);
if (retval < 0) {
retval = -errno;
@@ -1016,7 +1017,7 @@ static int process_requests(int sock)
retval = proxy_unmarshal(&in_iovec,
PROXY_HDR_SZ, "ss", &path, &name);
if (retval > 0) {
- retval = lremovexattr(path.data, name.data);
+ retval = qemu_lremovexattr(path.data, name.data);
if (retval < 0) {
retval = -errno;
}
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 1f0b1b0..7830526 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -776,16 +776,20 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
mode_t tmp_mode;
dev_t tmp_dev;
- if (fgetxattr(fd, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
+ if (qemu_fgetxattr(fd, "user.virtfs.uid",
+ &tmp_uid, sizeof(uid_t)) > 0) {
stbuf->st_uid = le32_to_cpu(tmp_uid);
}
- if (fgetxattr(fd, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
+ if (qemu_fgetxattr(fd, "user.virtfs.gid",
+ &tmp_gid, sizeof(gid_t)) > 0) {
stbuf->st_gid = le32_to_cpu(tmp_gid);
}
- if (fgetxattr(fd, "user.virtfs.mode", &tmp_mode, sizeof(mode_t)) > 0) {
+ if (qemu_fgetxattr(fd, "user.virtfs.mode",
+ &tmp_mode, sizeof(mode_t)) > 0) {
stbuf->st_mode = le32_to_cpu(tmp_mode);
}
- if (fgetxattr(fd, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
+ if (qemu_fgetxattr(fd, "user.virtfs.rdev",
+ &tmp_dev, sizeof(dev_t)) > 0) {
stbuf->st_rdev = le64_to_cpu(tmp_dev);
}
} else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 79ed6b2..50a03c7 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -19,6 +19,23 @@
#define O_PATH_9P_UTIL 0
#endif
+#ifdef CONFIG_DARWIN
+#define qemu_fgetxattr(...) fgetxattr(__VA_ARGS__, 0, 0)
+#define qemu_lgetxattr(...) getxattr(__VA_ARGS__, 0, XATTR_NOFOLLOW)
+#define qemu_llistxattr(...) listxattr(__VA_ARGS__, XATTR_NOFOLLOW)
+#define qemu_lremovexattr(...) removexattr(__VA_ARGS__, XATTR_NOFOLLOW)
+static inline int qemu_lsetxattr(const char *path, const char *name,
+ const void *value, size_t size, int flags) {
+ return setxattr(path, name, value, size, 0, flags | XATTR_NOFOLLOW);
+}
+#else
+#define qemu_fgetxattr fgetxattr
+#define qemu_lgetxattr lgetxattr
+#define qemu_llistxattr llistxattr
+#define qemu_lremovexattr lremovexattr
+#define qemu_lsetxattr lsetxattr
+#endif
+
static inline void close_preserve_errno(int fd)
{
int serrno = errno;
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 17/20] 9p: darwin: Provide a fallback implementation for utimensat
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (15 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 16/20] 9p: darwin: Compatibility for f/l*xattr Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 18/20] 9p: darwin: Implement compatibility for mknodat Keno Fischer
` (2 subsequent siblings)
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
This function is new in Mac OS 10.13. Provide a fallback implementation
when building against older SDKs. The complication in the definition comes
having to separately handle the used SDK version and the target OS version.
- If the SDK version is too low (__MAC_10_13 not defined), utimensat is not
defined in the header, so we must not try to use it (doing so would error).
- Otherwise, if the targetted OS version is at least 10.13, we know this
function is available, so we can unconditionally call it.
- Lastly, we check for the availability of the __builtin_available macro to
potentially insert a dynamic check for this OS version. However, __builtin_available
is only available with sufficiently recent versions of clang and while all
Apple clang versions that ship with Xcode versions that support the 10.13
SDK support with builtin, we want to allow building with compilers other
than Apple clang that may not support this builtin.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1:
* Correct calculation of tv_usec
* Support UTIME_NOW/UTIME/OMIT
* Now covers fsdev/virtfs-proxy-helper.c
fsdev/virtfs-proxy-helper.c | 3 +-
hw/9pfs/9p-local.c | 2 +-
hw/9pfs/9p-util-darwin.c | 96 +++++++++++++++++++++++++++++++++++++++++++++
hw/9pfs/9p-util-linux.c | 6 +++
hw/9pfs/9p-util.h | 8 ++++
hw/9pfs/9p.c | 1 +
6 files changed, 113 insertions(+), 3 deletions(-)
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index a26f8b8..d8dd3f5 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -957,8 +957,7 @@ static int process_requests(int sock)
&spec[0].tv_sec, &spec[0].tv_nsec,
&spec[1].tv_sec, &spec[1].tv_nsec);
if (retval > 0) {
- retval = utimensat(AT_FDCWD, path.data, spec,
- AT_SYMLINK_NOFOLLOW);
+ retval = utimensat_nofollow(AT_FDCWD, path.data, spec);
if (retval < 0) {
retval = -errno;
}
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7830526..47e8580 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1071,7 +1071,7 @@ static int local_utimensat(FsContext *s, V9fsPath *fs_path,
goto out;
}
- ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+ ret = utimensat_nofollow(dirfd, name, buf);
close_preserve_errno(dirfd);
out:
g_free(dirpath);
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index cdb4c9e..ac414bc 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -62,3 +62,99 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
close_preserve_errno(fd);
return ret;
}
+
+#ifndef __has_builtin
+#define __has_builtin(x) 0
+#endif
+
+static int update_times_from_stat(int fd, struct timespec times[2],
+ int update0, int update1)
+{
+ struct stat buf;
+ int ret = fstat(fd, &buf);
+ if (ret == -1) {
+ return ret;
+ }
+ if (update0) {
+ times[0] = buf.st_atimespec;
+ }
+ if (update1) {
+ times[1] = buf.st_mtimespec;
+ }
+ return 0;
+}
+
+int utimensat_nofollow(int dirfd, const char *filename,
+ const struct timespec times_in[2])
+{
+ int ret, fd;
+ int special0, special1;
+ struct timeval futimes_buf[2];
+ struct timespec times[2];
+ memcpy(times, times_in, 2 * sizeof(struct timespec));
+
+/* Check whether we have an SDK version that defines utimensat */
+#if defined(__MAC_10_13)
+# if __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_13
+# define UTIMENSAT_AVAILABLE 1
+# elif __has_builtin(__builtin_available)
+# define UTIMENSAT_AVAILABLE __builtin_available(macos 10.13, *)
+# else
+# define UTIMENSAT_AVAILABLE 0
+# endif
+ if (UTIMENSAT_AVAILABLE) {
+ return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+ }
+#endif
+
+ /* utimensat not available. Use futimes. */
+ fd = openat_file(dirfd, filename, O_PATH_9P_UTIL | O_NOFOLLOW, 0);
+ if (fd == -1) {
+ return -1;
+ }
+
+ special0 = times[0].tv_nsec == UTIME_OMIT;
+ special1 = times[1].tv_nsec == UTIME_OMIT;
+ if (special0 || special1) {
+ /* If both are set, nothing to do */
+ if (special0 && special1) {
+ ret = 0;
+ goto done;
+ }
+
+ ret = update_times_from_stat(fd, times, special0, special1);
+ if (ret < 0) {
+ goto done;
+ }
+ }
+
+ special0 = times[0].tv_nsec == UTIME_NOW;
+ special1 = times[1].tv_nsec == UTIME_NOW;
+ if (special0 || special1) {
+ ret = futimes(fd, NULL);
+ if (ret < 0) {
+ goto done;
+ }
+
+ /* If both are set, we are done */
+ if (special0 && special1) {
+ ret = 0;
+ goto done;
+ }
+
+ ret = update_times_from_stat(fd, times, special0, special1);
+ if (ret < 0) {
+ goto done;
+ }
+ }
+
+ futimes_buf[0].tv_sec = times[0].tv_sec;
+ futimes_buf[0].tv_usec = times[0].tv_nsec / 1000;
+ futimes_buf[1].tv_sec = times[1].tv_sec;
+ futimes_buf[1].tv_usec = times[1].tv_nsec / 1000;
+ ret = futimes(fd, futimes_buf);
+
+done:
+ close_preserve_errno(fd);
+ return ret;
+}
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
index defa3a4..3902378 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -57,3 +57,9 @@ int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
g_free(proc_path);
return ret;
}
+
+int utimensat_nofollow(int dirfd, const char *filename,
+ const struct timespec times[2])
+{
+ return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 50a03c7..b1dc08a 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -36,6 +36,12 @@ static inline int qemu_lsetxattr(const char *path, const char *name,
#define qemu_lsetxattr lsetxattr
#endif
+/* Compatibility with old SDK Versions for Darwin */
+#if defined(CONFIG_DARWIN) && !defined(UTIME_NOW)
+#define UTIME_NOW -1
+#define UTIME_OMIT -2
+#endif
+
static inline void close_preserve_errno(int fd)
{
int serrno = errno;
@@ -81,5 +87,7 @@ ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
char *list, size_t size);
ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
const char *name);
+int utimensat_nofollow(int dirfd, const char *filename,
+ const struct timespec times[2]);
#endif
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 24802b9..71b2dc9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -21,6 +21,7 @@
#include "virtio-9p.h"
#include "fsdev/qemu-fsdev.h"
#include "9p-xattr.h"
+#include "9p-util.h"
#include "coth.h"
#include "trace.h"
#include "migration/blocker.h"
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 18/20] 9p: darwin: Implement compatibility for mknodat
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (16 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 17/20] 9p: darwin: Provide a fallback implementation for utimensat Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 19/20] 9p: darwin: virtfs-proxy: Implement setuid code for darwin Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 20/20] 9p: darwin: configure: Allow VirtFS on Darwin Keno Fischer
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Darwin does not support mknodat. However, to avoid race conditions
with later setting the permissions, we must avoid using mknod on
the full path instead. We could try to fchdir, but that would cause
problems if multiple threads try to call mknodat at the same time.
However, luckily there is a solution: Darwin as an (unexposed in the
C library) system call that sets the cwd for the current thread only.
This should suffice to use mknod safely.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1: New patch. The previous series marked mknodat unsupported.
hw/9pfs/9p-local.c | 5 +++--
hw/9pfs/9p-util-darwin.c | 25 +++++++++++++++++++++++++
hw/9pfs/9p-util-linux.c | 5 +++++
hw/9pfs/9p-util.h | 2 ++
4 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 47e8580..c7a2b08 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -668,7 +668,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
- err = mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
+ err = qemu_mknodat(dirfd, name, fs_ctx->fmode | S_IFREG, 0);
if (err == -1) {
goto out;
}
@@ -683,7 +683,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
}
} else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
fs_ctx->export_flags & V9FS_SM_NONE) {
- err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
+ err = qemu_mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
if (err == -1) {
goto out;
}
@@ -696,6 +696,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
err_end:
unlinkat_preserve_errno(dirfd, name, 0);
+
out:
close_preserve_errno(dirfd);
return err;
diff --git a/hw/9pfs/9p-util-darwin.c b/hw/9pfs/9p-util-darwin.c
index ac414bc..49fe7d3 100644
--- a/hw/9pfs/9p-util-darwin.c
+++ b/hw/9pfs/9p-util-darwin.c
@@ -158,3 +158,28 @@ done:
close_preserve_errno(fd);
return ret;
}
+
+#ifndef SYS___pthread_fchdir
+# define SYS___pthread_fchdir 349
+#endif
+
+static int fchdir_thread_local(int fd)
+{
+ return syscall(SYS___pthread_fchdir, fd);
+}
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+ int preserved_errno, err;
+ if (fchdir_thread_local(dirfd) < 0) {
+ return -1;
+ }
+ err = mknod(filename, mode, dev);
+ preserved_errno = errno;
+ /* Stop using the thread-local cwd */
+ fchdir_thread_local(-1);
+ if (err < 0) {
+ errno = preserved_errno;
+ }
+ return err;
+}
diff --git a/hw/9pfs/9p-util-linux.c b/hw/9pfs/9p-util-linux.c
index 3902378..06399c5 100644
--- a/hw/9pfs/9p-util-linux.c
+++ b/hw/9pfs/9p-util-linux.c
@@ -63,3 +63,8 @@ int utimensat_nofollow(int dirfd, const char *filename,
{
return utimensat(dirfd, filename, times, AT_SYMLINK_NOFOLLOW);
}
+
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev)
+{
+ return mknodat(dirfd, filename, mode, dev);
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index b1dc08a..127564d 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -90,4 +90,6 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
int utimensat_nofollow(int dirfd, const char *filename,
const struct timespec times[2]);
+int qemu_mknodat(int dirfd, const char *filename, mode_t mode, dev_t dev);
+
#endif
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 19/20] 9p: darwin: virtfs-proxy: Implement setuid code for darwin
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (17 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 18/20] 9p: darwin: Implement compatibility for mknodat Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 20/20] 9p: darwin: configure: Allow VirtFS on Darwin Keno Fischer
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Darwin does not have linux capabilities, so make that code linux-only.
Darwin also does not have setresuid/gid. The correct way to temporarily
drop capabilities is to call seteuid/gid.
Also factor out the code that acquires acquire_dac_override into a separate
function in the linux implementation. I had originally done this when
I thought it made sense to have only one `setugid` function, but I retained
this because it seems clearer this way.
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1: New patch.
fsdev/virtfs-proxy-helper.c | 200 +++++++++++++++++++++++++++-----------------
1 file changed, 125 insertions(+), 75 deletions(-)
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index d8dd3f5..6baf2a6 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -82,6 +82,7 @@ static void do_perror(const char *string)
}
}
+#ifdef CONFIG_LINUX
static int do_cap_set(cap_value_t *cap_value, int size, int reset)
{
cap_t caps;
@@ -121,6 +122,85 @@ error:
return -1;
}
+static int acquire_dac_override(void)
+{
+ cap_value_t cap_list[] = {
+ CAP_DAC_OVERRIDE,
+ };
+ return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);
+}
+
+/*
+ * from man 7 capabilities, section
+ * Effect of User ID Changes on Capabilities:
+ * If the effective user ID is changed from nonzero to 0, then the permitted
+ * set is copied to the effective set. If the effective user ID is changed
+ * from 0 to nonzero, then all capabilities are are cleared from the effective
+ * set.
+ *
+ * The setfsuid/setfsgid man pages warn that changing the effective user ID may
+ * expose the program to unwanted signals, but this is not true anymore: for an
+ * unprivileged (without CAP_KILL) program to send a signal, the real or
+ * effective user ID of the sending process must equal the real or saved user
+ * ID of the target process. Even when dropping privileges, it is enough to
+ * keep the saved UID to a "privileged" value and virtfs-proxy-helper won't
+ * be exposed to signals. So just use setresuid/setresgid.
+ */
+static int setugid(int uid, int gid, int *suid, int *sgid)
+{
+ int retval;
+
+ *suid = geteuid();
+ *sgid = getegid();
+
+ if (setresgid(-1, gid, *sgid) == -1) {
+ retval = -errno;
+ goto err_out;
+ }
+
+ if (setresuid(-1, uid, *suid) == -1) {
+ retval = -errno;
+ goto err_sgid;
+ }
+
+ if (uid != 0 || gid != 0) {
+ /*
+ * We still need DAC_OVERRIDE because we don't change
+ * supplementary group ids, and hence may be subjected DAC rules
+ */
+ if (acquire_dac_override() < 0) {
+ retval = -errno;
+ goto err_suid;
+ }
+ }
+ return 0;
+
+err_suid:
+ if (setresuid(-1, *suid, *suid) == -1) {
+ abort();
+ }
+err_sgid:
+ if (setresgid(-1, *sgid, *sgid) == -1) {
+ abort();
+ }
+err_out:
+ return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+ if (setresgid(-1, sgid, sgid) == -1) {
+ abort();
+ }
+ if (setresuid(-1, suid, suid) == -1) {
+ abort();
+ }
+}
+
static int init_capabilities(void)
{
/* helper needs following capabilities only */
@@ -135,6 +215,51 @@ static int init_capabilities(void)
};
return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
}
+#else
+static int setugid(int uid, int gid, int *suid, int *sgid)
+{
+ int retval;
+
+ *suid = geteuid();
+ *sgid = getegid();
+
+ if (setegid(gid) == -1) {
+ retval = -errno;
+ goto err_out;
+ }
+
+ if (seteuid(uid) == -1) {
+ retval = -errno;
+ goto err_sgid;
+ }
+
+err_sgid:
+ if (setgid(*sgid) == -1) {
+ abort();
+ }
+err_out:
+ return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+ if (setegid(sgid) == -1) {
+ abort();
+ }
+ if (seteuid(suid) == -1) {
+ abort();
+ }
+}
+
+static int init_capabilities(void)
+{
+ return 0;
+}
+#endif
static int socket_read(int sockfd, void *buff, ssize_t size)
{
@@ -279,81 +404,6 @@ static int send_status(int sockfd, struct iovec *iovec, int status)
}
/*
- * from man 7 capabilities, section
- * Effect of User ID Changes on Capabilities:
- * If the effective user ID is changed from nonzero to 0, then the permitted
- * set is copied to the effective set. If the effective user ID is changed
- * from 0 to nonzero, then all capabilities are are cleared from the effective
- * set.
- *
- * The setfsuid/setfsgid man pages warn that changing the effective user ID may
- * expose the program to unwanted signals, but this is not true anymore: for an
- * unprivileged (without CAP_KILL) program to send a signal, the real or
- * effective user ID of the sending process must equal the real or saved user
- * ID of the target process. Even when dropping privileges, it is enough to
- * keep the saved UID to a "privileged" value and virtfs-proxy-helper won't
- * be exposed to signals. So just use setresuid/setresgid.
- */
-static int setugid(int uid, int gid, int *suid, int *sgid)
-{
- int retval;
-
- /*
- * We still need DAC_OVERRIDE because we don't change
- * supplementary group ids, and hence may be subjected DAC rules
- */
- cap_value_t cap_list[] = {
- CAP_DAC_OVERRIDE,
- };
-
- *suid = geteuid();
- *sgid = getegid();
-
- if (setresgid(-1, gid, *sgid) == -1) {
- retval = -errno;
- goto err_out;
- }
-
- if (setresuid(-1, uid, *suid) == -1) {
- retval = -errno;
- goto err_sgid;
- }
-
- if (uid != 0 || gid != 0) {
- if (do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0) < 0) {
- retval = -errno;
- goto err_suid;
- }
- }
- return 0;
-
-err_suid:
- if (setresuid(-1, *suid, *suid) == -1) {
- abort();
- }
-err_sgid:
- if (setresgid(-1, *sgid, *sgid) == -1) {
- abort();
- }
-err_out:
- return retval;
-}
-
-/*
- * This is used to reset the ugid back with the saved values
- * There is nothing much we can do checking error values here.
- */
-static void resetugid(int suid, int sgid)
-{
- if (setresgid(-1, sgid, sgid) == -1) {
- abort();
- }
- if (setresuid(-1, suid, suid) == -1) {
- abort();
- }
-}
-
-/*
* send response in two parts
* 1) ProxyHeader
* 2) Response or error status
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [Qemu-devel] [PATCH v2 20/20] 9p: darwin: configure: Allow VirtFS on Darwin
2018-06-01 1:25 [Qemu-devel] [PATCH v2 00/20] 9p: Add support for Darwin Keno Fischer
` (18 preceding siblings ...)
2018-06-01 1:26 ` [Qemu-devel] [PATCH v2 19/20] 9p: darwin: virtfs-proxy: Implement setuid code for darwin Keno Fischer
@ 2018-06-01 1:26 ` Keno Fischer
19 siblings, 0 replies; 34+ messages in thread
From: Keno Fischer @ 2018-06-01 1:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Keno Fischer, groug
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
Changes from v1: Now builds the proxy-helper on Darwin.
Makefile.objs | 1 +
configure | 22 +++++++++++++++-------
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index c6c3554..a2245c9 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -104,6 +104,7 @@ common-obj-$(CONFIG_WIN32) += os-win32.o
common-obj-$(CONFIG_POSIX) += os-posix.o
common-obj-$(CONFIG_LINUX) += fsdev/
+common-obj-$(CONFIG_DARWIN) += fsdev/
common-obj-y += migration/
diff --git a/configure b/configure
index a6a4616..4808459 100755
--- a/configure
+++ b/configure
@@ -5535,16 +5535,28 @@ if test "$want_tools" = "yes" ; then
fi
fi
if test "$softmmu" = yes ; then
- if test "$linux" = yes; then
- if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
+ if test "$virtfs" != no; then
+ if test "$linux" = yes; then
+ if test "$cap" = yes && test "$attr" = yes ; then
+ virtfs=yes
+ tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
+ else
+ if test "$virtfs" = yes; then
+ error_exit "VirtFS requires libcap devel and libattr devel under Linux"
+ fi
+ virtfs=no
+ fi
+ elif test "$darwin" = yes; then
virtfs=yes
tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
else
if test "$virtfs" = yes; then
- error_exit "VirtFS requires libcap devel and libattr devel"
+ error_exit "VirtFS is supported only on Linux and Darwin"
fi
virtfs=no
fi
+ fi
+ if test "$linux" = yes; then
if test "$mpath" != no && test "$mpathpersist" = yes ; then
mpath=yes
else
@@ -5555,10 +5567,6 @@ if test "$softmmu" = yes ; then
fi
tools="$tools scsi/qemu-pr-helper\$(EXESUF)"
else
- if test "$virtfs" = yes; then
- error_exit "VirtFS is supported only on Linux"
- fi
- virtfs=no
if test "$mpath" = yes; then
error_exit "Multipath is supported only on Linux"
fi
--
2.8.1
^ permalink raw reply related [flat|nested] 34+ messages in thread