| Message ID | 925f4c0a62291c070991d1b3e75a770de986a686.1391541706.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > In preparation for supporting reopen on gluster, move flag > parsing out to a function. Also, store open_flags and filename > in the gluster state storage struct, and add a NULL check in the > gconf cleanup. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/gluster.c | 48 ++++++++++++++++++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 16 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index a009b15..79af3fd 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB { > typedef struct BDRVGlusterState { > struct glfs *glfs; > struct glfs_fd *fd; > + int open_flags; > + char *filename; > } BDRVGlusterState; > > #define GLUSTER_FD_READ 0 > @@ -45,11 +47,13 @@ typedef struct GlusterConf { > > static void qemu_gluster_gconf_free(GlusterConf *gconf) > { > - g_free(gconf->server); > - g_free(gconf->volname); > - g_free(gconf->image); > - g_free(gconf->transport); > - g_free(gconf); > + if (gconf) { > + g_free(gconf->server); > + g_free(gconf->volname); > + g_free(gconf->image); > + g_free(gconf->transport); > + g_free(gconf); > + } > } > > static int parse_volume_options(GlusterConf *gconf, char *path) > @@ -269,11 +273,27 @@ static QemuOptsList runtime_opts = { > }, > }; > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > +{ > + assert(open_flags != NULL); > + > + *open_flags |= O_BINARY; > + > + if (bdrv_flags & BDRV_O_RDWR) { > + *open_flags |= O_RDWR; > + } else { > + *open_flags |= O_RDONLY; > + } > + > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > + *open_flags |= O_DIRECT; > + } > +} I saw the enable-O_SYNC option here. http://www.gluster.org/community/documentation/index.php/Translators/performance Why the gluster driver does not allow to enable O_SYNC ? > + > static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > int bdrv_flags, Error **errp) > { > BDRVGlusterState *s = bs->opaque; > - int open_flags = O_BINARY; > int ret = 0; > GlusterConf *gconf = g_malloc0(sizeof(GlusterConf)); > QemuOpts *opts; > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > > filename = qemu_opt_get(opts, "filename"); > > + s->filename = g_strdup(filename); > + > s->glfs = qemu_gluster_init(gconf, filename); > if (!s->glfs) { > ret = -errno; > goto out; > } > > - if (bdrv_flags & BDRV_O_RDWR) { > - open_flags |= O_RDWR; > - } else { > - open_flags |= O_RDONLY; > - } > - > - if ((bdrv_flags & BDRV_O_NOCACHE)) { > - open_flags |= O_DIRECT; > - } > + qemu_gluster_parse_flags(bdrv_flags, &s->open_flags); > > - s->fd = glfs_open(s->glfs, gconf->image, open_flags); > + s->fd = glfs_open(s->glfs, gconf->image, s->open_flags); > if (!s->fd) { > ret = -errno; > } > @@ -324,6 +338,7 @@ out: > if (s->glfs) { > glfs_fini(s->glfs); > } > + g_free(s->filename); > return ret; > } > > @@ -589,6 +604,7 @@ static void qemu_gluster_close(BlockDriverState *bs) > s->fd = NULL; > } > glfs_fini(s->glfs); > + g_free(s->filename); > } > > static int qemu_gluster_has_zero_init(BlockDriverState *bs) > -- > 1.8.3.1 > >
On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote: > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > > > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > > +{ > > + assert(open_flags != NULL); > > + > > + *open_flags |= O_BINARY; > > + > > + if (bdrv_flags & BDRV_O_RDWR) { > > + *open_flags |= O_RDWR; > > + } else { > > + *open_flags |= O_RDONLY; > > + } > > + > > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > > + *open_flags |= O_DIRECT; > > + } > > +} > > I saw the enable-O_SYNC option here. > http://www.gluster.org/community/documentation/index.php/Translators/performance > Why the gluster driver does not allow to enable O_SYNC ? I am not aware of any option in QEMU (like cache= etc) that will force block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ? Turning off write-behind for the entire gluster volume isn't an option ? Regards, Bharata.
Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit : > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote: > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > > > > > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > > > +{ > > > + assert(open_flags != NULL); > > > + > > > + *open_flags |= O_BINARY; > > > + > > > + if (bdrv_flags & BDRV_O_RDWR) { > > > + *open_flags |= O_RDWR; > > > + } else { > > > + *open_flags |= O_RDONLY; > > > + } > > > + > > > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > > > + *open_flags |= O_DIRECT; > > > + } > > > +} > > > > I saw the enable-O_SYNC option here. > > http://www.gluster.org/community/documentation/index.php/Translators/performance > > Why the gluster driver does not allow to enable O_SYNC ? > > I am not aware of any option in QEMU (like cache= etc) that will force > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ? [,cache=writethrough|writeback|none|directsync|unsafe][ I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC. Best regards Benoît > > Turning off write-behind for the entire gluster volume isn't an option ? > > Regards, > Bharata. >
On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote: > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit : > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote: > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > > > > > > > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > > > > +{ > > > > + assert(open_flags != NULL); > > > > + > > > > + *open_flags |= O_BINARY; > > > > + > > > > + if (bdrv_flags & BDRV_O_RDWR) { > > > > + *open_flags |= O_RDWR; > > > > + } else { > > > > + *open_flags |= O_RDONLY; > > > > + } > > > > + > > > > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > > > > + *open_flags |= O_DIRECT; > > > > + } > > > > +} > > > > > > I saw the enable-O_SYNC option here. > > > http://www.gluster.org/community/documentation/index.php/Translators/performance > > > Why the gluster driver does not allow to enable O_SYNC ? > > > > I am not aware of any option in QEMU (like cache= etc) that will force > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ? > > [,cache=writethrough|writeback|none|directsync|unsafe][ > > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC. May be I am missing something, but I checked the flags with which raw protocol driver opens the file image for different cache options and this is what I found by looking at open flags in util/osdep.c:qemu_open() (Ignoring O_CLOEXEC which is common for all the cases) writethrough 02 O_RDWR writeback 02 O_RDWR none 040002 O_DIRECT|O_RDWR directsync 040002 O_DIRECT|O_RDWR unsafe 02 O_RDWR I do see the below comment in block/raw-posix.c:raw_parse_flags() /* Use O_DSYNC for write-through caching, no flags for write-back caching, * and O_DIRECT for no caching. */ if ((bdrv_flags & BDRV_O_NOCACHE)) { *open_flags |= O_DIRECT; } But I don't see the driver actually setting O_DSYNC anywhere. Regards, Bharata.
On Fri, Feb 07, 2014 at 08:57:35PM +0530, Bharata B Rao wrote: > On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote: > > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit : > > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote: > > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > > > > > > > > > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > > > > > +{ > > > > > + assert(open_flags != NULL); > > > > > + > > > > > + *open_flags |= O_BINARY; > > > > > + > > > > > + if (bdrv_flags & BDRV_O_RDWR) { > > > > > + *open_flags |= O_RDWR; > > > > > + } else { > > > > > + *open_flags |= O_RDONLY; > > > > > + } > > > > > + > > > > > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > > > > > + *open_flags |= O_DIRECT; > > > > > + } > > > > > +} > > > > > > > > I saw the enable-O_SYNC option here. > > > > http://www.gluster.org/community/documentation/index.php/Translators/performance > > > > Why the gluster driver does not allow to enable O_SYNC ? > > > > > > I am not aware of any option in QEMU (like cache= etc) that will force > > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ? > > > > [,cache=writethrough|writeback|none|directsync|unsafe][ > > > > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC. > > May be I am missing something, but I checked the flags with which > raw protocol driver opens the file image for different cache options > and this is what I found by looking at open flags in util/osdep.c:qemu_open() > > (Ignoring O_CLOEXEC which is common for all the cases) > > writethrough 02 O_RDWR > writeback 02 O_RDWR > none 040002 O_DIRECT|O_RDWR > directsync 040002 O_DIRECT|O_RDWR > unsafe 02 O_RDWR > > I do see the below comment in block/raw-posix.c:raw_parse_flags() > > /* Use O_DSYNC for write-through caching, no flags for write-back caching, > * and O_DIRECT for no caching. */ > if ((bdrv_flags & BDRV_O_NOCACHE)) { > *open_flags |= O_DIRECT; > } > > But I don't see the driver actually setting O_DSYNC anywhere. > You are not mistaken. For qemu writethrough cache (the default), it is not via O_SYNC/DSYNC directly in the protocol drivers, but essentially done inside block.c (e.g., bdrv_flush()).
Le Friday 07 Feb 2014 à 20:57:35 (+0530), Bharata B Rao a écrit : > On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote: > > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit : > > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote: > > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > > > > > > > > > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > > > > > +{ > > > > > + assert(open_flags != NULL); > > > > > + > > > > > + *open_flags |= O_BINARY; > > > > > + > > > > > + if (bdrv_flags & BDRV_O_RDWR) { > > > > > + *open_flags |= O_RDWR; > > > > > + } else { > > > > > + *open_flags |= O_RDONLY; > > > > > + } > > > > > + > > > > > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > > > > > + *open_flags |= O_DIRECT; > > > > > + } > > > > > +} > > > > > > > > I saw the enable-O_SYNC option here. > > > > http://www.gluster.org/community/documentation/index.php/Translators/performance > > > > Why the gluster driver does not allow to enable O_SYNC ? > > > > > > I am not aware of any option in QEMU (like cache= etc) that will force > > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ? > > > > [,cache=writethrough|writeback|none|directsync|unsafe][ > > > > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC. > > May be I am missing something, but I checked the flags with which > raw protocol driver opens the file image for different cache options > and this is what I found by looking at open flags in util/osdep.c:qemu_open() > > (Ignoring O_CLOEXEC which is common for all the cases) > > writethrough 02 O_RDWR > writeback 02 O_RDWR > none 040002 O_DIRECT|O_RDWR > directsync 040002 O_DIRECT|O_RDWR > unsafe 02 O_RDWR > > I do see the below comment in block/raw-posix.c:raw_parse_flags() > > /* Use O_DSYNC for write-through caching, no flags for write-back caching, > * and O_DIRECT for no caching. */ > if ((bdrv_flags & BDRV_O_NOCACHE)) { > *open_flags |= O_DIRECT; > } > > But I don't see the driver actually setting O_DSYNC anywhere. True, Maybe it has changed over time. Best regards Benoît > > Regards, > Bharata. > >
Le Friday 07 Feb 2014 à 10:57:33 (-0500), Jeff Cody a écrit : > On Fri, Feb 07, 2014 at 08:57:35PM +0530, Bharata B Rao wrote: > > On Fri, Feb 07, 2014 at 03:22:29PM +0100, Benoît Canet wrote: > > > Le Friday 07 Feb 2014 à 09:14:50 (+0530), Bharata B Rao a écrit : > > > > On Wed, Feb 05, 2014 at 08:25:36PM +0100, Benoît Canet wrote: > > > > > Le Tuesday 04 Feb 2014 à 14:26:58 (-0500), Jeff Cody a écrit : > > > > > > > > > > > > +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) > > > > > > +{ > > > > > > + assert(open_flags != NULL); > > > > > > + > > > > > > + *open_flags |= O_BINARY; > > > > > > + > > > > > > + if (bdrv_flags & BDRV_O_RDWR) { > > > > > > + *open_flags |= O_RDWR; > > > > > > + } else { > > > > > > + *open_flags |= O_RDONLY; > > > > > > + } > > > > > > + > > > > > > + if ((bdrv_flags & BDRV_O_NOCACHE)) { > > > > > > + *open_flags |= O_DIRECT; > > > > > > + } > > > > > > +} > > > > > > > > > > I saw the enable-O_SYNC option here. > > > > > http://www.gluster.org/community/documentation/index.php/Translators/performance > > > > > Why the gluster driver does not allow to enable O_SYNC ? > > > > > > > > I am not aware of any option in QEMU (like cache= etc) that will force > > > > block driver (like gluster) to use O_SYNC. Do other drivers use O_SYNC ? > > > > > > [,cache=writethrough|writeback|none|directsync|unsafe][ > > > > > > I think writethough is O_SYNC and directsync is O_DIRECT|O_SYNC. > > > > May be I am missing something, but I checked the flags with which > > raw protocol driver opens the file image for different cache options > > and this is what I found by looking at open flags in util/osdep.c:qemu_open() > > > > (Ignoring O_CLOEXEC which is common for all the cases) > > > > writethrough 02 O_RDWR > > writeback 02 O_RDWR > > none 040002 O_DIRECT|O_RDWR > > directsync 040002 O_DIRECT|O_RDWR > > unsafe 02 O_RDWR > > > > I do see the below comment in block/raw-posix.c:raw_parse_flags() > > > > /* Use O_DSYNC for write-through caching, no flags for write-back caching, > > * and O_DIRECT for no caching. */ > > if ((bdrv_flags & BDRV_O_NOCACHE)) { > > *open_flags |= O_DIRECT; > > } > > > > But I don't see the driver actually setting O_DSYNC anywhere. > > > > You are not mistaken. For qemu writethrough cache (the default), it > is not via O_SYNC/DSYNC directly in the protocol drivers, but > essentially done inside block.c (e.g., bdrv_flush()). > > Hmm sorry for spreading fud. It was not intentional.
On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > diff --git a/block/gluster.c b/block/gluster.c > index a009b15..79af3fd 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB { > typedef struct BDRVGlusterState { > struct glfs *glfs; > struct glfs_fd *fd; > + int open_flags; Is this field used? I only see stores to this field but no loads. Seems unnecessary since the block layer already provides us with QEMU BDRV_* flags and qemu_gluster_parse_flags() can be used to produce POSIX open(2) flags from them.
On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > > filename = qemu_opt_get(opts, "filename"); > > + s->filename = g_strdup(filename); It's not obvious to me that copying the filename is necessary. block/raw-posix.c does this: raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags); Why didn't you use bs->filename? Stefan
On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote: > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > > > > filename = qemu_opt_get(opts, "filename"); > > > > + s->filename = g_strdup(filename); > > It's not obvious to me that copying the filename is necessary. > block/raw-posix.c does this: > > raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags); > > Why didn't you use bs->filename? > Back when the raw-posix reopen was implemented, the .bdrv_file_open used the char * filename instead of options parameters. Now that the filename is explicitly parsed via the options, I thought it made logical sense to cache the filename into the protocol state variable, for later use by reopen. That way if the bs->filename was changed in some manner, the protocol would have the variable as originally intended to be passed to the .bdrv_file_open() method. In reality, I should be able to just use bs->filename instead, as it does not get modified; this just seemed to make a bit more sense.
On Fri, Feb 14, 2014 at 03:12:41PM +0100, Stefan Hajnoczi wrote: > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > > diff --git a/block/gluster.c b/block/gluster.c > > index a009b15..79af3fd 100644 > > --- a/block/gluster.c > > +++ b/block/gluster.c > > @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB { > > typedef struct BDRVGlusterState { > > struct glfs *glfs; > > struct glfs_fd *fd; > > + int open_flags; > > Is this field used? I only see stores to this field but no loads. > > Seems unnecessary since the block layer already provides us with QEMU > BDRV_* flags and qemu_gluster_parse_flags() can be used to produce POSIX > open(2) flags from them. It is not currently used, I cached it for later use, but never used it. I will purge it in a v2 (same thing with the sister variable in the reopen state struct in patch 2).
On Fri, Feb 14, 2014 at 09:41:46AM -0500, Jeff Cody wrote: > On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote: > > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > > > > > > filename = qemu_opt_get(opts, "filename"); > > > > > > + s->filename = g_strdup(filename); > > > > It's not obvious to me that copying the filename is necessary. > > block/raw-posix.c does this: > > > > raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags); > > > > Why didn't you use bs->filename? > > > > Back when the raw-posix reopen was implemented, the .bdrv_file_open > used the char * filename instead of options parameters. Now that the > filename is explicitly parsed via the options, I thought it made > logical sense to cache the filename into the protocol state variable, > for later use by reopen. > > That way if the bs->filename was changed in some manner, the protocol > would have the variable as originally intended to be passed to the > .bdrv_file_open() method. > > In reality, I should be able to just use bs->filename instead, as it > does not get modified; this just seemed to make a bit more sense. I thought about driver-specific options that are not part of 'filename', too. IMO it's simpler to use bs->filename until we actually use driver-specific options, and then switch over to using bs->options instead of copying.
On Fri, Feb 14, 2014 at 04:38:03PM +0100, Stefan Hajnoczi wrote: > On Fri, Feb 14, 2014 at 09:41:46AM -0500, Jeff Cody wrote: > > On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote: > > > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > > > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > > > > > > > > filename = qemu_opt_get(opts, "filename"); > > > > > > > > + s->filename = g_strdup(filename); > > > > > > It's not obvious to me that copying the filename is necessary. > > > block/raw-posix.c does this: > > > > > > raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags); > > > > > > Why didn't you use bs->filename? > > > > > > > Back when the raw-posix reopen was implemented, the .bdrv_file_open > > used the char * filename instead of options parameters. Now that the > > filename is explicitly parsed via the options, I thought it made > > logical sense to cache the filename into the protocol state variable, > > for later use by reopen. > > > > That way if the bs->filename was changed in some manner, the protocol > > would have the variable as originally intended to be passed to the > > .bdrv_file_open() method. > > > > In reality, I should be able to just use bs->filename instead, as it > > does not get modified; this just seemed to make a bit more sense. > > I thought about driver-specific options that are not part of 'filename', > too. IMO it's simpler to use bs->filename until we actually use > driver-specific options, and then switch over to using bs->options > instead of copying. > OK, I'll make that change for v2 as well.
Am 14.02.2014 um 15:41 hat Jeff Cody geschrieben: > On Fri, Feb 14, 2014 at 03:21:28PM +0100, Stefan Hajnoczi wrote: > > On Tue, Feb 04, 2014 at 02:26:58PM -0500, Jeff Cody wrote: > > > @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > > > > > > filename = qemu_opt_get(opts, "filename"); > > > > > > + s->filename = g_strdup(filename); > > > > It's not obvious to me that copying the filename is necessary. > > block/raw-posix.c does this: > > > > raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags); > > > > Why didn't you use bs->filename? > > Back when the raw-posix reopen was implemented, the .bdrv_file_open > used the char * filename instead of options parameters. Now that the > filename is explicitly parsed via the options, I thought it made > logical sense to cache the filename into the protocol state variable, > for later use by reopen. > > That way if the bs->filename was changed in some manner, the protocol > would have the variable as originally intended to be passed to the > .bdrv_file_open() method. > > In reality, I should be able to just use bs->filename instead, as it > does not get modified; this just seemed to make a bit more sense. FWIW, I think we want bs->filename to go away eventually and get it replaced by a callback (or perhaps two, a short summary one for humans and a complete one) so that the callers work with setups that are driven by options and don't have a filename. Kevin
diff --git a/block/gluster.c b/block/gluster.c index a009b15..79af3fd 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -30,6 +30,8 @@ typedef struct GlusterAIOCB { typedef struct BDRVGlusterState { struct glfs *glfs; struct glfs_fd *fd; + int open_flags; + char *filename; } BDRVGlusterState; #define GLUSTER_FD_READ 0 @@ -45,11 +47,13 @@ typedef struct GlusterConf { static void qemu_gluster_gconf_free(GlusterConf *gconf) { - g_free(gconf->server); - g_free(gconf->volname); - g_free(gconf->image); - g_free(gconf->transport); - g_free(gconf); + if (gconf) { + g_free(gconf->server); + g_free(gconf->volname); + g_free(gconf->image); + g_free(gconf->transport); + g_free(gconf); + } } static int parse_volume_options(GlusterConf *gconf, char *path) @@ -269,11 +273,27 @@ static QemuOptsList runtime_opts = { }, }; +static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) +{ + assert(open_flags != NULL); + + *open_flags |= O_BINARY; + + if (bdrv_flags & BDRV_O_RDWR) { + *open_flags |= O_RDWR; + } else { + *open_flags |= O_RDONLY; + } + + if ((bdrv_flags & BDRV_O_NOCACHE)) { + *open_flags |= O_DIRECT; + } +} + static int qemu_gluster_open(BlockDriverState *bs, QDict *options, int bdrv_flags, Error **errp) { BDRVGlusterState *s = bs->opaque; - int open_flags = O_BINARY; int ret = 0; GlusterConf *gconf = g_malloc0(sizeof(GlusterConf)); QemuOpts *opts; @@ -291,23 +311,17 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, filename = qemu_opt_get(opts, "filename"); + s->filename = g_strdup(filename); + s->glfs = qemu_gluster_init(gconf, filename); if (!s->glfs) { ret = -errno; goto out; } - if (bdrv_flags & BDRV_O_RDWR) { - open_flags |= O_RDWR; - } else { - open_flags |= O_RDONLY; - } - - if ((bdrv_flags & BDRV_O_NOCACHE)) { - open_flags |= O_DIRECT; - } + qemu_gluster_parse_flags(bdrv_flags, &s->open_flags); - s->fd = glfs_open(s->glfs, gconf->image, open_flags); + s->fd = glfs_open(s->glfs, gconf->image, s->open_flags); if (!s->fd) { ret = -errno; } @@ -324,6 +338,7 @@ out: if (s->glfs) { glfs_fini(s->glfs); } + g_free(s->filename); return ret; } @@ -589,6 +604,7 @@ static void qemu_gluster_close(BlockDriverState *bs) s->fd = NULL; } glfs_fini(s->glfs); + g_free(s->filename); } static int qemu_gluster_has_zero_init(BlockDriverState *bs)
In preparation for supporting reopen on gluster, move flag parsing out to a function. Also, store open_flags and filename in the gluster state storage struct, and add a NULL check in the gconf cleanup. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/gluster.c | 48 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 16 deletions(-)