On 07/31/2012 01:17 PM, Eric Blake wrote:
> On 07/30/2012 03:34 PM, Supriya Kannery wrote:
>> raw-posix driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>
>> Signed-off-by: Supriya Kannery <supri...@linux.vnet.ibm.com>
>>
>> ---
>> Index: qemu/block/raw.c
>> ===================================================================
>
>> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
>> + int flags)
>> +{
>> + BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
>> + BDRVRawState *s = bs->opaque;
>> + int ret = 0;
>> +
>> + raw_rs->reopen_state.bs = bs;
>> +
>> + /* stash state before reopen */
>> + raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
>> + raw_stash_state(raw_rs->stash_s, s);
>> + s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
>
> You called it out in your cover letter, but just pointing out that this
> is one of the locations that needs a conditional fallback to
> dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing.
>
> More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and
> NOT dup3, so that you are duplicating to the first available fd rather
> than accidentally overwriting whatever s->fd happened to be on entry.
> Also, where is your error checking that you didn't just assign s->fd to
> -1? It looks like your goal here is to stash a copy of the fd, so that
> on failure you can then pivot over to your copy.
In addition, as it is written above will always assign -1 to s->fd, and
stash_s->fd every time, with errno "Bad file descriptor".
The function raw_stash_state() does not save s->fd, but instead sets the
stashed copy (raw_rs->stash_s->fd) to be -1.
Then this:
s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
assigns s->fd to -1.
I don't think dup3() will work at all, because if -1 is used for one of the
file descriptors, that is EBADF, and if stash_s->fd == s->fd, that is
EINVAL.
So I agree, I think we definitely want fcntl(F_DUP_CLOEXEC) here.
>
>> +
>> + *prs = &(raw_rs->reopen_state);
>> +
>> + /* Flags that can be set using fcntl */
>> + int fcntl_flags = BDRV_O_NOCACHE;
>
> This variable name is misleading; fcntl_flags in my mind implies O_* not
> BDRV_O_*, as we are not guaranteed that they are the same values. Maybe
> bdrv_flags is a better name. Or are you trying to list the subset of
> BDRV_O flags that can be changed via mapping to the appropriate O_ flag
> during fcntl?
>
>> +
>> + if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) {
>> + if ((flags & BDRV_O_NOCACHE)) {
>> + s->open_flags |= O_DIRECT;
>> + } else {
>> + s->open_flags &= ~O_DIRECT;
>> + }
>> + ret = fcntl_setfl(s->fd, s->open_flags);
>> + } else {
>> +
>> + /* close and reopen using new flags */
>> + bs->drv->bdrv_close(bs);
>> + ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
>> + }
>
> At any rate, in spite of the poor choice of naming for fcntl_flags, the
> logic here looked correct - if the only BDRV_O_ bits that changed
> between existing flags and the requested flags can be handled by fcntl,
> then use fcntl to change them, otherwise open from scratch.
>
>> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
>> +{
>> + BDRVRawReopenState *raw_rs;
>> + BDRVRawState *s = bs->opaque;
>> +
>> + raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
>> +
>> + /* revert to stashed state */
>> + if (s->fd != -1) {
>> + close(s->fd);
>> + }
>
> At first, I worried that you needed to use dup3() here to restore s->fd
> from the cached fd...
>
>> + raw_revert_state(s, raw_rs->stash_s);
>
> then I read the code for raw_revert_state, where you are rewriting the
> state to permanently use the dup'd copy rather than trying to revert
> back to the old fd number. As long as we are sure that all other points
> in the code base always go through s->fd rather than caching the old
> value, and therefore don't have a stale reference, then swapping the
> struct instead of using dup3 to restore the exact fd value we previously
> had should work.
>
