RE: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

RE: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[Aaron Mason]

there

sent from my Telstra NEXTG™ handset

-----Original Message-----
From: Kevin Wolf <kwolf@redhat.com>
Sent: Wednesday, 4 August 2010 10:29 PM
To: andrzej zaborowski <balrogg@gmail.com>
Cc: Aaron Mason <aaron.mason@thats-too-much.info>; qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

Am 04.08.2010 14:27, schrieb andrzej zaborowski:
> Hi,
> 
> On 4 August 2010 12:30, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 04.08.2010 01:46, schrieb Aaron Mason:
>>> +    const char *real_filename, *temp_str, *adapterType = "ide";
> 
> Sorry to complain about style, but note that uppercase characters are
> not used in variable names in Qemu (that I see).

Whoops, missed that one when complaining about the other style problems.
Yes, this should be adapter_type.

Kevin

Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[Aaron Mason]

Let's try that again.

I'd paste it but GMail likes to break patches to honour the 73 char
limit.  I'll install Git on my home PC and try git send-email from
there (my work PC is behind a restrictive firewall so email via Gmail
is out).

Thanks for finding more that I missed, I'll fix them on Monday next
time I'm in the office.

On Thu, Aug 5, 2010 at 7:07 AM, Aaron Mason
<aaron.mason@thats-too-much.info> wrote:
> there
>
> sent from my Telstra NEXTG™ handset
>
> -----Original Message-----
> From: Kevin Wolf <kwolf@redhat.com>
> Sent: Wednesday, 4 August 2010 10:29 PM
> To: andrzej zaborowski <balrogg@gmail.com>
> Cc: Aaron Mason <aaron.mason@thats-too-much.info>; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type
>
> Am 04.08.2010 14:27, schrieb andrzej zaborowski:
>> Hi,
>>
>> On 4 August 2010 12:30, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 04.08.2010 01:46, schrieb Aaron Mason:
>>>> +    const char *real_filename, *temp_str, *adapterType = "ide";
>>
>> Sorry to complain about style, but note that uppercase characters are
>> not used in variable names in Qemu (that I see).
>
> Whoops, missed that one when complaining about the other style problems.
> Yes, this should be adapter_type.
>
> Kevin
>

[Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[Aaron Mason]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

Hi,

Now that I have half a clue, please find attached a properly formatted
patch for the above with a signed-off line.  Hopefully attaching it
won't cause issues as I have winblows on this machine and can't get
git send-email to work at this time.

Regards

[-- Attachment #2: 0001-Added-an-option-to-set-the-VMDK-adapter-type.patch --]
[-- Type: application/octet-stream, Size: 2956 bytes --]

From 37bed87750573b7ac737c3f2a919b68a06a00513 Mon Sep 17 00:00:00 2001
From: unknown <aaron.mason@.rlpb.org.au>
Date: Wed, 4 Aug 2010 08:41:38 +1000
Subject: [PATCH] Added an option to set the VMDK adapter type

Signed-off-by: Aaron Mason <aaron.mason@thats-too-much.info>
---
 block/vmdk.c |   20 +++++++++++++++++---
 block_int.h  |    1 +
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 2d4ba42..ef7733d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -686,9 +686,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
         "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
         "ddb.geometry.heads = \"16\"\n"
         "ddb.geometry.sectors = \"63\"\n"
-        "ddb.adapterType = \"ide\"\n";
+        "ddb.adapterType = \"%s\"\n";
     char desc[1024];
-    const char *real_filename, *temp_str;
+    const char *real_filename, *temp_str, *adapterType = "ide";
     int64_t total_size = 0;
     const char *backing_file = NULL;
     int flags = 0;
@@ -702,6 +702,15 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
             backing_file = options->value.s;
         } else if (!strcmp(options->name, BLOCK_OPT_COMPAT6)) {
             flags |= options->value.n ? BLOCK_FLAG_COMPAT6: 0;
+        } else if (!strcmp(options->name, BLOCK_OPT_ADAPTER)) {
+            if (options->value.s != NULL) {
+                if (!strcmp(options->value.s, "ide") || 
+                    !strcmp(options->value.s, "buslogic") ||
+                    !strcmp(options->value.s, "lsilogic")) {
+                    adapterType = options->value.s;
+                } else
+                    return -1;
+            }
         }
         options++;
     }
@@ -799,7 +808,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
     snprintf(desc, sizeof(desc), desc_template, (unsigned int)time(NULL),
              total_size, real_filename,
              (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
-             total_size / (int64_t)(63 * 16));
+             total_size / (int64_t)(63 * 16), adapterType);
 
     /* write the descriptor */
     lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
@@ -845,6 +854,11 @@ static QEMUOptionParameter vmdk_create_options[] = {
         .type = OPT_FLAG,
         .help = "VMDK version 6 image"
     },
+    {
+        .name = BLOCK_OPT_ADAPTER,
+        .type = OPT_STRING,
+        .help = "Adapter type - ide (default), lsilogic or buslogic"
+    },
     { NULL }
 };
 
diff --git a/block_int.h b/block_int.h
index 96ff4cf..ec90de0 100644
--- a/block_int.h
+++ b/block_int.h
@@ -39,6 +39,7 @@
 #define BLOCK_OPT_BACKING_FMT   "backing_fmt"
 #define BLOCK_OPT_CLUSTER_SIZE  "cluster_size"
 #define BLOCK_OPT_PREALLOC      "preallocation"
+#define BLOCK_OPT_ADAPTER       "adapter_type"
 
 typedef struct AIOPool {
     void (*cancel)(BlockDriverAIOCB *acb);
-- 
1.7.0.2.msysgit.0

Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[Natalia Portillo]

Please resend it as inline code (pasted) not as an attachment.

Thanks

El 04/08/2010, a las 00:46, Aaron Mason escribió:

> Hi,
> 
> Now that I have half a clue, please find attached a properly formatted
> patch for the above with a signed-off line.  Hopefully attaching it
> won't cause issues as I have winblows on this machine and can't get
> git send-email to work at this time.
> 
> Regards
> <0001-Added-an-option-to-set-the-VMDK-adapter-type.patch>

Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[Kevin Wolf]

Hi Aaron,

Am 04.08.2010 01:46, schrieb Aaron Mason:
> Now that I have half a clue, please find attached a properly formatted
> patch for the above with a signed-off line.  Hopefully attaching it
> won't cause issues as I have winblows on this machine and can't get
> git send-email to work at this time.

Works for me, though it would be even better to attach it for applying
and inlining it additionally to make it easier to quote.

One more round of changes and I think we've got it:

> From 37bed87750573b7ac737c3f2a919b68a06a00513 Mon Sep 17 00:00:00 2001
> From: unknown <aaron.mason@.rlpb.org.au>

You should tell git your real name and mail address in its config file.
You can use git commit --amend --reset-author to update the existing commit.

> Date: Wed, 4 Aug 2010 08:41:38 +1000
> Subject: [PATCH] Added an option to set the VMDK adapter type
> 
> Signed-off-by: Aaron Mason <aaron.mason@thats-too-much.info>
> ---
>  block/vmdk.c |   20 +++++++++++++++++---
>  block_int.h  |    1 +
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2d4ba42..ef7733d 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -686,9 +686,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
>          "ddb.geometry.cylinders = \"%" PRId64 "\"\n"
>          "ddb.geometry.heads = \"16\"\n"
>          "ddb.geometry.sectors = \"63\"\n"
> -        "ddb.adapterType = \"ide\"\n";
> +        "ddb.adapterType = \"%s\"\n";
>      char desc[1024];
> -    const char *real_filename, *temp_str;
> +    const char *real_filename, *temp_str, *adapterType = "ide";
>      int64_t total_size = 0;
>      const char *backing_file = NULL;
>      int flags = 0;
> @@ -702,6 +702,15 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
>              backing_file = options->value.s;
>          } else if (!strcmp(options->name, BLOCK_OPT_COMPAT6)) {
>              flags |= options->value.n ? BLOCK_FLAG_COMPAT6: 0;
> +        } else if (!strcmp(options->name, BLOCK_OPT_ADAPTER)) {
> +            if (options->value.s != NULL) {
> +                if (!strcmp(options->value.s, "ide") || 

This line has trailing whitespace.

> +                    !strcmp(options->value.s, "buslogic") ||
> +                    !strcmp(options->value.s, "lsilogic")) {
> +                    adapterType = options->value.s;
> +                } else
> +                    return -1;

Please add braces even for single statements, see CODING_STYLE.

Also better return -EINVAL as the return value is used as a negative
errno to produce the error message.

Kevin

Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[andrzej zaborowski]

Hi,

On 4 August 2010 12:30, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 04.08.2010 01:46, schrieb Aaron Mason:
>> +    const char *real_filename, *temp_str, *adapterType = "ide";

Sorry to complain about style, but note that uppercase characters are
not used in variable names in Qemu (that I see).

Cheers

Re: [Qemu-devel] [PATCH] Added an option to set the VMDK adapter type

[Kevin Wolf]

Am 04.08.2010 14:27, schrieb andrzej zaborowski:
> Hi,
> 
> On 4 August 2010 12:30, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 04.08.2010 01:46, schrieb Aaron Mason:
>>> +    const char *real_filename, *temp_str, *adapterType = "ide";
> 
> Sorry to complain about style, but note that uppercase characters are
> not used in variable names in Qemu (that I see).

Whoops, missed that one when complaining about the other style problems.
Yes, this should be adapter_type.

Kevin