[Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Christoph Hellwig]

Add a -g flag to the open command and the main qemu-io command line to
allow opening a file growable.  This is only allowed for protocols,
mirroring the limitation exposed through bdrv_file_open.


Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: qemu/qemu-io.c
===================================================================
--- qemu.orig/qemu-io.c	2009-07-04 17:42:16.505239002 +0200
+++ qemu/qemu-io.c	2009-07-04 17:52:26.432241152 +0200
@@ -1172,7 +1172,7 @@ static const cmdinfo_t close_cmd = {
 	.oneline	= "close the current open file",
 };
 
-static int openfile(char *name, int flags)
+static int openfile(char *name, int flags, int growable)
 {
 	if (bs) {
 		fprintf(stderr, "file open already, try 'help close'\n");
@@ -1189,6 +1189,16 @@ static int openfile(char *name, int flag
 		return 1;
 	}
 
+
+	if (growable) {
+		if (!bs->drv || !bs->drv->protocol_name) {
+			fprintf(stderr,
+				"%s: only protocols can be opened growable\n",
+				progname);
+		}
+		bs->growable = 1;
+	}
+
 	return 0;
 }
 
@@ -1207,6 +1217,7 @@ open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache\n"
+" -g, -- allow file to grow (only applies to protocols)"
 "\n");
 }
 
@@ -1217,9 +1228,10 @@ open_f(int argc, char **argv)
 {
 	int flags = 0;
 	int readonly = 0;
+	int growable = 0;
 	int c;
 
-	while ((c = getopt(argc, argv, "snCr")) != EOF) {
+	while ((c = getopt(argc, argv, "snCrg")) != EOF) {
 		switch (c) {
 		case 's':
 			flags |= BDRV_O_SNAPSHOT;
@@ -1233,6 +1245,9 @@ open_f(int argc, char **argv)
 		case 'r':
 			readonly = 1;
 			break;
+		case 'g':
+			growable = 1;
+			break;
 		default:
 			return command_usage(&open_cmd);
 		}
@@ -1246,7 +1261,7 @@ open_f(int argc, char **argv)
 	if (optind != argc - 1)
 		return command_usage(&open_cmd);
 
-	return openfile(argv[optind], flags);
+	return openfile(argv[optind], flags, growable);
 }
 
 static const cmdinfo_t open_cmd = {
@@ -1306,7 +1321,8 @@ static void usage(const char *name)
 int main(int argc, char **argv)
 {
 	int readonly = 0;
-	const char *sopt = "hVc:Crsnm";
+	int growable = 0;
+	const char *sopt = "hVc:Crsnmg";
 	struct option lopt[] = {
 		{ "help", 0, 0, 'h' },
 		{ "version", 0, 0, 'V' },
@@ -1317,6 +1333,7 @@ int main(int argc, char **argv)
 		{ "snapshot", 0, 0, 's' },
 		{ "nocache", 0, 0, 'n' },
 		{ "misalign", 0, 0, 'm' },
+		{ "growable", 0, 0, 'g' },
 		{ NULL, 0, 0, 0 }
 	};
 	int c;
@@ -1345,6 +1362,9 @@ int main(int argc, char **argv)
 		case 'm':
 			misalign = 1;
 			break;
+		case 'g':
+			growable = 1;
+			break;
 		case 'V':
 			printf("%s version %s\n", progname, VERSION);
 			exit(0);
@@ -1392,7 +1412,7 @@ int main(int argc, char **argv)
 		flags |= BDRV_O_RDWR;
 
 	if ((argc - optind) == 1)
-		openfile(argv[optind], flags);
+		openfile(argv[optind], flags, growable);
 	command_loop();
 
 	/*

Re: [Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Kevin Wolf]

Christoph Hellwig schrieb:
> Add a -g flag to the open command and the main qemu-io command line to
> allow opening a file growable.  This is only allowed for protocols,
> mirroring the limitation exposed through bdrv_file_open.

This limitation doesn't look quite right. I'm not sure if growable
images are used anywhere else, too, but their main usage is for the
image file access in formats like qcow - that is, growable images are
usually raw. And raw didn't have a protocol name until recently.


> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> 
> Index: qemu/qemu-io.c
> ===================================================================
> --- qemu.orig/qemu-io.c	2009-07-04 17:42:16.505239002 +0200
> +++ qemu/qemu-io.c	2009-07-04 17:52:26.432241152 +0200
> @@ -1172,7 +1172,7 @@ static const cmdinfo_t close_cmd = {
>  	.oneline	= "close the current open file",
>  };
>  
> -static int openfile(char *name, int flags)
> +static int openfile(char *name, int flags, int growable)
>  {
>  	if (bs) {
>  		fprintf(stderr, "file open already, try 'help close'\n");
> @@ -1189,6 +1189,16 @@ static int openfile(char *name, int flag
>  		return 1;
>  	}
>  
> +
> +	if (growable) {
> +		if (!bs->drv || !bs->drv->protocol_name) {
> +			fprintf(stderr,
> +				"%s: only protocols can be opened growable\n",
> +				progname);
> +		}
> +		bs->growable = 1;

But you don't seem to enforce the limitation anyway, so in the end
everything works. ;-)

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Christoph Hellwig]

On Mon, Jul 06, 2009 at 10:53:53AM +0200, Kevin Wolf wrote:
> Christoph Hellwig schrieb:
> > Add a -g flag to the open command and the main qemu-io command line to
> > allow opening a file growable.  This is only allowed for protocols,
> > mirroring the limitation exposed through bdrv_file_open.
> 
> This limitation doesn't look quite right. I'm not sure if growable
> images are used anywhere else, too, but their main usage is for the
> image file access in formats like qcow - that is, growable images are
> usually raw. And raw didn't have a protocol name until recently.

It does have a protocol now, and the way current qemu is implemented
growable is only set from bdrv_file_open, which implies the BDRV_O_FILE
argument to bdrv_open2, which means we use find_protocol to find the
BlockDriver and find_protocol needs protocol_name set.

If you want to backport this patch to a qemu version that doesn't
have a protocol_name for raw you'll have to adjust it to whatever check
is correct in that version.

Re: [Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Kevin Wolf]

Christoph Hellwig schrieb:
> On Mon, Jul 06, 2009 at 10:53:53AM +0200, Kevin Wolf wrote:
>> Christoph Hellwig schrieb:
>>> Add a -g flag to the open command and the main qemu-io command line to
>>> allow opening a file growable.  This is only allowed for protocols,
>>> mirroring the limitation exposed through bdrv_file_open.
>> This limitation doesn't look quite right. I'm not sure if growable
>> images are used anywhere else, too, but their main usage is for the
>> image file access in formats like qcow - that is, growable images are
>> usually raw. And raw didn't have a protocol name until recently.
> 
> It does have a protocol now, and the way current qemu is implemented
> growable is only set from bdrv_file_open, which implies the BDRV_O_FILE
> argument to bdrv_open2, which means we use find_protocol to find the
> BlockDriver and find_protocol needs protocol_name set.
> 
> If you want to backport this patch to a qemu version that doesn't
> have a protocol_name for raw you'll have to adjust it to whatever check
> is correct in that version.

Ok, you are right. raw was the only exception because it could be used
as a protocol even though it didn't have a protocol name.

Looks fine to me then if you enforce the limitation instead of only
printing an error message and opening the image anyway. Not printing the
error and opening the image would be fine either. I just don't like
error messages without errors.

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Christoph Hellwig]

On Mon, Jul 06, 2009 at 12:40:06PM +0200, Kevin Wolf wrote:
> Ok, you are right. raw was the only exception because it could be used
> as a protocol even though it didn't have a protocol name.
> 
> Looks fine to me then if you enforce the limitation instead of only
> printing an error message and opening the image anyway. Not printing the
> error and opening the image would be fine either. I just don't like
> error messages without errors.

Well, the primary use of qemu-iotests is to try out the I/O patterns,
and the -g option is to check out we do the correct thing for growable
files, so I'd really prefer it to fail if we're not allow to actually
make it growable.

Now one think I could do is to just add a growable flag to the
BlockDriver to make it explicit.  Currently only raw files (in the posix
and win32 flavours) would set it.  That would also fix the bug what we
currenly allow to grow host devices even if we can't.

Re: [Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Kevin Wolf]

Christoph Hellwig schrieb:
> On Mon, Jul 06, 2009 at 12:40:06PM +0200, Kevin Wolf wrote:
>> Ok, you are right. raw was the only exception because it could be used
>> as a protocol even though it didn't have a protocol name.
>>
>> Looks fine to me then if you enforce the limitation instead of only
>> printing an error message and opening the image anyway. Not printing the
>> error and opening the image would be fine either. I just don't like
>> error messages without errors.
> 
> Well, the primary use of qemu-iotests is to try out the I/O patterns,
> and the -g option is to check out we do the correct thing for growable
> files, so I'd really prefer it to fail if we're not allow to actually
> make it growable.

Perfectly reasonable. But then make it really fail.

> Now one think I could do is to just add a growable flag to the
> BlockDriver to make it explicit.  Currently only raw files (in the posix
> and win32 flavours) would set it.

Good point actually, your current logic is wrong: raw-win32 doesn't have
the file protocol. I'm not sure though if introducing a new flag just
for qemu-io is right. After all qemu-io should test what the drivers
provide to qemu and not extend them to work with qemu-io. Currently the
drivers don't have the concept of non-growable images.

>  That would also fix the bug what we
> currenly allow to grow host devices even if we can't.

How that? The host device drivers don't have a protocol name, do they?

Kevin

Re: [Qemu-devel] [PATCH] qemu-io: add flag to mark files growable

[Christoph Hellwig]

On Mon, Jul 06, 2009 at 01:04:55PM +0200, Kevin Wolf wrote:
> > Well, the primary use of qemu-iotests is to try out the I/O patterns,
> > and the -g option is to check out we do the correct thing for growable
> > files, so I'd really prefer it to fail if we're not allow to actually
> > make it growable.
> 
> Perfectly reasonable. But then make it really fail.

Oh, now I see your complaint.  The lack of a return 1 after the error
message was not intentional - I've added it now.

> > Now one think I could do is to just add a growable flag to the
> > BlockDriver to make it explicit.  Currently only raw files (in the posix
> > and win32 flavours) would set it.
> 
> Good point actually, your current logic is wrong: raw-win32 doesn't have
> the file protocol. I'm not sure though if introducing a new flag just
> for qemu-io is right. After all qemu-io should test what the drivers
> provide to qemu and not extend them to work with qemu-io. Currently the
> drivers don't have the concept of non-growable images.

True.  I'll stick to the current one with the added error return.

win32 raw should probably grow a protocol name, but I'll leave that to
people who can actually test win32.

> >  That would also fix the bug what we
> > currenly allow to grow host devices even if we can't.
> 
> How that? The host device drivers don't have a protocol name, do they?

They don't, I was confused.

Updated patch below:


Index: qemu/qemu-io.c
===================================================================
--- qemu.orig/qemu-io.c	2009-07-06 13:09:45.223364426 +0200
+++ qemu/qemu-io.c	2009-07-06 13:11:41.347269140 +0200
@@ -1172,7 +1172,7 @@ static const cmdinfo_t close_cmd = {
 	.oneline	= "close the current open file",
 };
 
-static int openfile(char *name, int flags)
+static int openfile(char *name, int flags, int growable)
 {
 	if (bs) {
 		fprintf(stderr, "file open already, try 'help close'\n");
@@ -1189,6 +1189,17 @@ static int openfile(char *name, int flag
 		return 1;
 	}
 
+
+	if (growable) {
+		if (!bs->drv || !bs->drv->protocol_name) {
+			fprintf(stderr,
+				"%s: only protocols can be opened growable\n",
+				progname);
+			return 1;
+		}
+		bs->growable = 1;
+	}
+
 	return 0;
 }
 
@@ -1207,6 +1218,7 @@ open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache\n"
+" -g, -- allow file to grow (only applies to protocols)"
 "\n");
 }
 
@@ -1217,9 +1229,10 @@ open_f(int argc, char **argv)
 {
 	int flags = 0;
 	int readonly = 0;
+	int growable = 0;
 	int c;
 
-	while ((c = getopt(argc, argv, "snCr")) != EOF) {
+	while ((c = getopt(argc, argv, "snCrg")) != EOF) {
 		switch (c) {
 		case 's':
 			flags |= BDRV_O_SNAPSHOT;
@@ -1233,6 +1246,9 @@ open_f(int argc, char **argv)
 		case 'r':
 			readonly = 1;
 			break;
+		case 'g':
+			growable = 1;
+			break;
 		default:
 			return command_usage(&open_cmd);
 		}
@@ -1246,7 +1262,7 @@ open_f(int argc, char **argv)
 	if (optind != argc - 1)
 		return command_usage(&open_cmd);
 
-	return openfile(argv[optind], flags);
+	return openfile(argv[optind], flags, growable);
 }
 
 static const cmdinfo_t open_cmd = {
@@ -1306,7 +1322,8 @@ static void usage(const char *name)
 int main(int argc, char **argv)
 {
 	int readonly = 0;
-	const char *sopt = "hVc:Crsnm";
+	int growable = 0;
+	const char *sopt = "hVc:Crsnmg";
 	struct option lopt[] = {
 		{ "help", 0, 0, 'h' },
 		{ "version", 0, 0, 'V' },
@@ -1317,6 +1334,7 @@ int main(int argc, char **argv)
 		{ "snapshot", 0, 0, 's' },
 		{ "nocache", 0, 0, 'n' },
 		{ "misalign", 0, 0, 'm' },
+		{ "growable", 0, 0, 'g' },
 		{ NULL, 0, 0, 0 }
 	};
 	int c;
@@ -1345,6 +1363,9 @@ int main(int argc, char **argv)
 		case 'm':
 			misalign = 1;
 			break;
+		case 'g':
+			growable = 1;
+			break;
 		case 'V':
 			printf("%s version %s\n", progname, VERSION);
 			exit(0);
@@ -1392,7 +1413,7 @@ int main(int argc, char **argv)
 		flags |= BDRV_O_RDWR;
 
 	if ((argc - optind) == 1)
-		openfile(argv[optind], flags);
+		openfile(argv[optind], flags, growable);
 	command_loop();
 
 	/*