thr3ads.net - Xen devel - [xen-devel][PATCH 0/5] xl disk configuration handling [Feb 2011]

If this information is useful, please help other people find it:
Share via:

Kamala Narasimhan

2011-Feb-07 21:15 UTC

[xen-devel][PATCH 0/5] xl disk configuration handling

Refactor xl disk configuration handling.

Patch 1/5 - "Xl Disk Configuration Option" documentation
Patch 2/5 - Xl interface change plus changes to code it impacts
Patch 3/5 - Xl disk configuration parsing changes
Patch 4/5 - Xl block-attach refactoring to reuse xl parsing code
Patch 5/5 - More disk validation checks

Note:  I am nervous about this patch series as the changes are huge and I am
worried about regressions especially this close to 4.1.  I can try and fix the
regressions fast but only after they are found and reported.  So, I would
encourage folks to run their standard config files that they knew worked before
against the latest changes and report any/all regressions.  Thanks!

Kamala

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-09 18:21 UTC

head link

[xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Description:

This patch refactors xl disk specific interface to separate disk format and
backend types, rename some variables, changes code that is impacted by this
interface change.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

PS:  This patch incorporates all the comments made so far that are specific to
this patch.




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-09 18:26 UTC

head link

Re: [xen-devel][PATCH 0/5] xl disk configuration handling

Kamala Narasimhan wrote:> Refactor xl disk configuration handling.
> 
> Patch 1/5 - "Xl Disk Configuration Option" documentation
> Patch 2/5 - Xl interface change plus changes to code it impacts
> Patch 3/5 - Xl disk configuration parsing changes
> Patch 4/5 - Xl block-attach refactoring to reuse xl parsing code
> Patch 5/5 - More disk validation checks
> 
> Note:  I am nervous about this patch series as the changes are huge and I
am
> worried about regressions especially this close to 4.1.  I can try and fix
the
> regressions fast but only after they are found and reported.  So, I would
> encourage folks to run their standard config files that they knew worked
before
> against the latest changes and report any/all regressions.  Thanks!
> Note: Upon further discussion a decision was made to go with patches 1 & 2
for
4.1 and the rest will be reposted for 4.2 and would include relevant review
comments.

Kamala



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2011-Feb-10 09:23 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

> -static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
> libxl_disk_phystype disk_type)
> +static int validate_virtual_disk(libxl_ctx *ctx, char *file_name, 
> +    libxl_disk_backend backend_type, libxl_disk_format format)
>  {
>      struct stat stat_buf;
>  
> -    if ( (file_name[0] == ''\0'') && (disk_type ==
PHYSTYPE_EMPTY) )
> +    if ((file_name[0] == ''\0'') && (format ==
DISK_FORMAT_EMPTY))
>          return 0;
If format == DISK_FORMAT_EMPTY then does the content of file_name
matter?

Alternatively, if format == DISK_FORMAT_EMPTY then is file_name != ""
actually an error?

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2011-Feb-10 11:50 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

On Wed, 9 Feb 2011, Kamala Narasimhan wrote:> Description:
> 
> This patch refactors xl disk specific interface to separate disk format and
backend types, rename some variables, changes code that is impacted by this
interface change.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> PS:  This patch incorporates all the comments made so far that are specific
to this patch.
Better, we are almost there.
The only problems I have found are in the changes to
libxl_device_disk_local_attach:

> @@ -1052,36 +1051,44 @@ char * libxl_device_disk_local_attach(li
>      libxl__gc gc = LIBXL_INIT_GC(ctx);
>      const char *dev = NULL;
>      char *ret = NULL;
> -    int phystype = disk->phystype;
> -    switch (phystype) {
> -        case PHYSTYPE_PHY: {
> -            fprintf(stderr, "attaching PHY disk %s to domain
0\n", disk->physpath);
> -            dev = disk->physpath;
> +
> +    switch (disk->backend) {
> +        case DISK_BACKEND_PHY: {
> +            fprintf(stderr, "attaching PHY disk %s to domain
0\n", disk->pdev_path);
> +            dev = disk->pdev_path;
>              break;
>          }
> -        case PHYSTYPE_FILE:
> -            /* let''s pretend is tap:aio for the moment */
> -            phystype = PHYSTYPE_AIO;
> -        case PHYSTYPE_AIO:
> -            if (!libxl__blktap_enabled(&gc)) {
> -                dev = disk->physpath;
> +        case DISK_BACKEND_TAP: {
> +            if (disk->format == DISK_FORMAT_VHD)
> +            {
> +                if (libxl__blktap_enabled(&gc))
> +                    dev = libxl__blktap_devpath(&gc,
disk->pdev_path, disk->format);
> +                else
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is
required to open a vhd disk\n");
> +                break;
> +            } else if (disk->format == DISK_FORMAT_QCOW ||
> +                       disk->format == DISK_FORMAT_QCOW2) {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally
attach a qcow or qcow2 disk image\n");
> +                break;
> +            } else {
> +                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend "
> +                    "type: %d\n", disk->backend);
>                  break;
>              }
> -        case PHYSTYPE_VHD:
> -            if (libxl__blktap_enabled(&gc))
> -                dev = libxl__blktap_devpath(&gc, disk->physpath,
phystype);
> -            else
> -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapdisk2 is
required to open a vhd disk\n");
> +        }
> +        case DISK_BACKEND_QDISK: {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach
a qdisk "
> +                "image\n");
>              break;
> -        case PHYSTYPE_QCOW:
> -        case PHYSTYPE_QCOW2:
> -            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach
a qcow or qcow2 disk image\n");
> +        }
> +        case DISK_BACKEND_UNKNOWN:
> +        default: {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend "
> +                "type: %d\n", disk->backend);
>              break;
> +        }
> +    }
>  
> -        default:
> -            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
physical type: %d\n", phystype);
> -            break;
> -    }
>      if (dev != NULL)
>          ret = strdup(dev);
>      libxl__free_all(&gc);
> 
The simple raw file case is not supported anywhere, besides it is
possible to attach a qdisk if it is in raw format.
It should be more or less like this:


phy && raw -> OK
phy && anything but raw -> KO
qdisk && raw -> OK
qdisk && anything but raw -> KO
tap && (qcow || qcow2) -> KO
tap && blktap2 enabled && (vhd || raw) -> OK
tap && blktap2 disabled && vhd -> KO
tap && blktap2 disabled && raw -> OK (same as qdisk)

> diff -r 1967c7c290eb tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed Feb 09 12:03:09 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c	Wed Feb 09 10:57:42 2011 -0500
> @@ -361,9 +361,9 @@ static void printf_info(int domid,
>          printf("\t\t(tap\n");
>          printf("\t\t\t(backend_domid %d)\n",
d_config->disks[i].backend_domid);
>          printf("\t\t\t(domid %d)\n",
d_config->disks[i].domid);
> -        printf("\t\t\t(physpath %s)\n",
d_config->disks[i].physpath);
> -        printf("\t\t\t(phystype %d)\n",
d_config->disks[i].phystype);
> -        printf("\t\t\t(virtpath %s)\n",
d_config->disks[i].virtpath);
> +        printf("\t\t\t(physpath %s)\n",
d_config->disks[i].pdev_path);
> +        printf("\t\t\t(phystype %d)\n",
d_config->disks[i].backend);
> +        printf("\t\t\t(virtpath %s)\n",
d_config->disks[i].vdev);
> 
we should print pdev_path, backend and vdev here

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-10 14:44 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Ian Campbell wrote:>> -static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
>> libxl_disk_phystype disk_type)
>> +static int validate_virtual_disk(libxl_ctx *ctx, char *file_name, 
>> +    libxl_disk_backend backend_type, libxl_disk_format format)
>>  {
>>      struct stat stat_buf;
>>  
>> -    if ( (file_name[0] == ''\0'') &&
(disk_type == PHYSTYPE_EMPTY) )
>> +    if ((file_name[0] == ''\0'') && (format ==
DISK_FORMAT_EMPTY))
>>          return 0;
> 
> If format == DISK_FORMAT_EMPTY then does the content of file_name
> matter?
>Good catch!  Just checking for format should do.  I will make that change.
> Alternatively, if format == DISK_FORMAT_EMPTY then is file_name !=
""
> actually an error?
> Ideally we should never end up that way as our code checks the string before
setting format to DISK_FORMAT_EMPTY.

Kamala

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-10 17:05 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Stefano - Attached is patch 2/5 with the change you had asked.  Please let me
know if there are further ones you would suggest.

Description:

This patch refactors xl disk specific interface to separate disk format and
backend types, rename some variables, changes code that is impacted by this
interface change.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-10 20:00 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Stefano - This includes a fix for empty cdrom case issue you found while
testing.

Description:

This patch refactors xl disk specific interface to separate disk format and
backend types, rename some variables, changes code that is impacted by this
interface change.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2011-Feb-11 13:47 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

On Thu, 10 Feb 2011, Kamala Narasimhan wrote:> Stefano - This includes a fix for empty cdrom case issue you found while
testing.
> 
> Description:
> 
> This patch refactors xl disk specific interface to separate disk format and
backend types, rename some variables, changes code that is impacted by this
interface change.
> 
you need to update the ocaml bindings, otherwise the patch will cause
build problems:

make[7]: Entering directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
 CC       xl_stubs.o
xl_stubs.c: In function ''device_disk_val'':
xl_stubs.c:195: error: ''libxl_device_disk'' has no member named
''physpath''
xl_stubs.c:196: error: ''libxl_device_disk'' has no member named
''phystype''
xl_stubs.c:196: error: ''PHYSTYPE_QCOW'' undeclared (first use
in this function)
xl_stubs.c:196: error: (Each undeclared identifier is reported only once
xl_stubs.c:196: error: for each function it appears in.)
xl_stubs.c:197: error: ''libxl_device_disk'' has no member named
''virtpath''
make[7]: *** [xl_stubs.o] Error 1
make[7]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
make[6]: *** [subdir-install-xl] Error 2
make[6]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
make[5]: *** [subdirs-install] Error 2
make[5]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
make[4]: *** [subdir-install-libs] Error 2
make[4]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
make[3]: *** [subdirs-install] Error 2
make[3]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
make[2]: *** [subdir-install-ocaml] Error 2
make[2]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
make[1]: *** [subdirs-install] Error 2
make[1]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
make: *** [install-tools] Error 2


apart from this it looks OK to me.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-11 14:45 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Stefano Stabellini wrote:> On Thu, 10 Feb 2011, Kamala Narasimhan wrote:
>> Stefano - This includes a fix for empty cdrom case issue you found
while testing.
>>
>> Description:
>>
>> This patch refactors xl disk specific interface to separate disk format
and backend types, rename some variables, changes code that is impacted by this
interface change.
>>
> 
> you need to update the ocaml bindings, otherwise the patch will cause
> build problems:
> 
> make[7]: Entering directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
>  CC       xl_stubs.o
> xl_stubs.c: In function ''device_disk_val'':
> xl_stubs.c:195: error: ''libxl_device_disk'' has no member
named ''physpath''
> xl_stubs.c:196: error: ''libxl_device_disk'' has no member
named ''phystype''
> xl_stubs.c:196: error: ''PHYSTYPE_QCOW'' undeclared (first
use in this function)
> xl_stubs.c:196: error: (Each undeclared identifier is reported only once
> xl_stubs.c:196: error: for each function it appears in.)
> xl_stubs.c:197: error: ''libxl_device_disk'' has no member
named ''virtpath''
> make[7]: *** [xl_stubs.o] Error 1
> make[7]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
> make[6]: *** [subdir-install-xl] Error 2
> make[6]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
> make[5]: *** [subdirs-install] Error 2
> make[5]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
> make[4]: *** [subdir-install-libs] Error 2
> make[4]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
> make[3]: *** [subdirs-install] Error 2
> make[3]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
> make[2]: *** [subdir-install-ocaml] Error 2
> make[2]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
> make[1]: *** [subdirs-install] Error 2
> make[1]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
> make: *** [install-tools] Error 2
> 
> 
> apart from this it looks OK to me.
Turns out I didn''t have ocaml installed on my build system and skipped
past
ocaml builds and that is why I didn''t even see this issue.  I will fix
it.

Kamala


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-11 15:19 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Stefano Stabellini wrote:> On Thu, 10 Feb 2011, Kamala Narasimhan wrote:
>> Stefano - This includes a fix for empty cdrom case issue you found
while testing.
>>
>> Description:
>>
>> This patch refactors xl disk specific interface to separate disk format
and backend types, rename some variables, changes code that is impacted by this
interface change.
>>
> 
> you need to update the ocaml bindings, otherwise the patch will cause
> build problems:
> 
> make[7]: Entering directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
>  CC       xl_stubs.o
> xl_stubs.c: In function ''device_disk_val'':
> xl_stubs.c:195: error: ''libxl_device_disk'' has no member
named ''physpath''
> xl_stubs.c:196: error: ''libxl_device_disk'' has no member
named ''phystype''
> xl_stubs.c:196: error: ''PHYSTYPE_QCOW'' undeclared (first
use in this function)
> xl_stubs.c:196: error: (Each undeclared identifier is reported only once
> xl_stubs.c:196: error: for each function it appears in.)
> xl_stubs.c:197: error: ''libxl_device_disk'' has no member
named ''virtpath''
> make[7]: *** [xl_stubs.o] Error 1
> make[7]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
> make[6]: *** [subdir-install-xl] Error 2
> make[6]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
> make[5]: *** [subdirs-install] Error 2
> make[5]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
> make[4]: *** [subdir-install-libs] Error 2
> make[4]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
> make[3]: *** [subdirs-install] Error 2
> make[3]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
> make[2]: *** [subdir-install-ocaml] Error 2
> make[2]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
> make[1]: *** [subdirs-install] Error 2
> make[1]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
> make: *** [install-tools] Error 2
> 
> 
> apart from this it looks OK to me.
Stefano - Here is a revised patch that fixes the ocaml bindings build issues. 
While I confirmed that the build now works, I wouldn''t even know where
to start when it comes to testing it!

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2011-Feb-11 15:26 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

On Fri, 11 Feb 2011, Kamala Narasimhan wrote:> Stefano Stabellini wrote:
> > On Thu, 10 Feb 2011, Kamala Narasimhan wrote:
> >> Stefano - This includes a fix for empty cdrom case issue you found
while testing.
> >>
> >> Description:
> >>
> >> This patch refactors xl disk specific interface to separate disk
format and backend types, rename some variables, changes code that is impacted
by this interface change.
> >>
> > 
> > you need to update the ocaml bindings, otherwise the patch will cause
> > build problems:
> > 
> > make[7]: Entering directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
> >  CC       xl_stubs.o
> > xl_stubs.c: In function ''device_disk_val'':
> > xl_stubs.c:195: error: ''libxl_device_disk'' has no
member named ''physpath''
> > xl_stubs.c:196: error: ''libxl_device_disk'' has no
member named ''phystype''
> > xl_stubs.c:196: error: ''PHYSTYPE_QCOW'' undeclared
(first use in this function)
> > xl_stubs.c:196: error: (Each undeclared identifier is reported only
once
> > xl_stubs.c:196: error: for each function it appears in.)
> > xl_stubs.c:197: error: ''libxl_device_disk'' has no
member named ''virtpath''
> > make[7]: *** [xl_stubs.o] Error 1
> > make[7]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
> > make[6]: *** [subdir-install-xl] Error 2
> > make[6]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
> > make[5]: *** [subdirs-install] Error 2
> > make[5]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
> > make[4]: *** [subdir-install-libs] Error 2
> > make[4]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
> > make[3]: *** [subdirs-install] Error 2
> > make[3]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
> > make[2]: *** [subdir-install-ocaml] Error 2
> > make[2]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
> > make[1]: *** [subdirs-install] Error 2
> > make[1]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
> > make: *** [install-tools] Error 2
> > 
> > 
> > apart from this it looks OK to me.
> 
> Stefano - Here is a revised patch that fixes the ocaml bindings build
issues.  While I confirmed that the build now works, I wouldn''t even
know where to start when it comes to testing it!
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
The patch is OK for me.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-11 19:12 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Kamala Narasimhan wrote:> Stefano Stabellini wrote:
>> On Thu, 10 Feb 2011, Kamala Narasimhan wrote:
>>> Stefano - This includes a fix for empty cdrom case issue you found
while testing.
>>>
>>> Description:
>>>
>>> This patch refactors xl disk specific interface to separate disk
format and backend types, rename some variables, changes code that is impacted
by this interface change.
>>>
>> you need to update the ocaml bindings, otherwise the patch will cause
>> build problems:
>>
>> make[7]: Entering directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
>>  CC       xl_stubs.o
>> xl_stubs.c: In function ''device_disk_val'':
>> xl_stubs.c:195: error: ''libxl_device_disk'' has no
member named ''physpath''
>> xl_stubs.c:196: error: ''libxl_device_disk'' has no
member named ''phystype''
>> xl_stubs.c:196: error: ''PHYSTYPE_QCOW'' undeclared
(first use in this function)
>> xl_stubs.c:196: error: (Each undeclared identifier is reported only
once
>> xl_stubs.c:196: error: for each function it appears in.)
>> xl_stubs.c:197: error: ''libxl_device_disk'' has no
member named ''virtpath''
>> make[7]: *** [xl_stubs.o] Error 1
>> make[7]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs/xl''
>> make[6]: *** [subdir-install-xl] Error 2
>> make[6]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
>> make[5]: *** [subdirs-install] Error 2
>> make[5]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml/libs''
>> make[4]: *** [subdir-install-libs] Error 2
>> make[4]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
>> make[3]: *** [subdirs-install] Error 2
>> make[3]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools/ocaml''
>> make[2]: *** [subdir-install-ocaml] Error 2
>> make[2]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
>> make[1]: *** [subdirs-install] Error 2
>> make[1]: Leaving directory
`/local/scratch/sstabellini/xen-unstable.hg/tools''
>> make: *** [install-tools] Error 2
>>
>>
>> apart from this it looks OK to me.
> 
> Stefano - Here is a revised patch that fixes the ocaml bindings build
issues.  While I confirmed that the build now works, I wouldn''t even
know where to start when it comes to testing it!
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> Per suggestion, along with the latest patch is a more detailed description to
add to the check-in message -

Currently we pile all the backend and format information pertaining to disk
option in a single enum.  This check-in separates the two and uses two enums,
one for disk format and another for disk backend.  This helps clearly
differentiate between disk format and backend within the implementation and also
helps cleanup the code in this area in preparation for the impending parser
revamping to be done post 4.1.  Along with separating format and backend, this
check-in also removes unwanted types and renames variables in the disk interface
and fixes the code affected by the interface changes.

In specific, here are the disk interface changes made - In libxl_device_disk
structure physpath was renamed to pdev_path, virtpath was renamed to vdev,
phystype was removed and replaced with backend and format enums.  Also
previously a single enum libxl_disk_phystype held the values for qcow, qcow2,
vhd, aio, file, phy, empty and that got refactored into two enums,
libxl_disk_format to hold unknown, qcow, qcow2, vhd, raw, empty and
libxl_disk_backend to hold unknown, phy, tap and qdisk.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2011-Feb-14 17:45 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface change
plus changes to code it impacts"):> Per suggestion, along with the latest patch is a more detailed
> description to add to the check-in message -
It looks good to me, thanks, apart from some nits.  If you could
address these I''ll apply it :-).
> -            if (libxl__blktap_enabled(&gc))
> +            if (libxl__blktap_enabled(&gc) && 
> +                 disk->format != DISK_BACKEND_QDISK)
Some mistake here surely ?
> diff -r 6c22ae0f6540 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Fri Feb 11 16:51:44 2011 +0000
> +++ b/tools/libxl/libxl.c	Fri Feb 11 13:48:12 2011 -0500
...> -    switch (disk->phystype) {
...> +    switch (disk->backend) {
You have introduced these braces { }, but that seems to me to be just
a stylistic change and they are not really needed ?
> +            libxl__device_physdisk_major_minor(disk->pdev_path,
&major, &minor);
This function can fail.  It has two call sites neither of which check
the return value.  Perhaps that should be addressed in a separate
patch, as your change here isn''t making it worse.  So no need to do
anything about this right now if you don''t want to.
> -        case PHYSTYPE_QCOW2:
> -        case PHYSTYPE_VHD:
> +        case DISK_BACKEND_TAP:
> +        case DISK_BACKEND_QDISK: {
Once again, this is a stylistic change.  These { } are not used
elsewhere in libxl so you should not introduce them.  (It would be a
different matter if there were some reason why they are required in a
particular case, eg to introduce a relevant block scope.)
>          case DSTATE_PHYSPATH:
> -            if ( *p == '','' ) {
> +            if (*p == '','') {
>                  int ioemu_len;
It is good that you fixed up the formatting in code you changed, but
please do not make formatting changes to unchanged lines.

We will do a style cleanup after 4.1 is released.
> -                if ( tok + ioemu_len < end &&
> +                if (tok + ioemu_len < end &&
Once again.  Can you make sure your patch doesn''t have /any/ unrelated
formatting changes to lines you don''t touch, please ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-14 18:30 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Ian Jackson wrote:> Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface
change plus changes to code it impacts"):
>> Per suggestion, along with the latest patch is a more detailed
>> description to add to the check-in message -
> 
> It looks good to me, thanks, apart from some nits.  If you could
> address these I''ll apply it :-).
> 
>> -            if (libxl__blktap_enabled(&gc))
>> +            if (libxl__blktap_enabled(&gc) && 
>> +                 disk->format != DISK_BACKEND_QDISK)
> 
> Some mistake here surely ?
>Of course!
>> diff -r 6c22ae0f6540 tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.c	Fri Feb 11 16:51:44 2011 +0000
>> +++ b/tools/libxl/libxl.c	Fri Feb 11 13:48:12 2011 -0500
> ...
>> -    switch (disk->phystype) {
> ...
>> +    switch (disk->backend) {
> 
> You have introduced these braces { }, but that seems to me to be just
> a stylistic change and they are not really needed ?
> No, I have switched disk->phystype to disk->backend.
>> +            libxl__device_physdisk_major_minor(disk->pdev_path,
&major, &minor);
> 
> This function can fail.  It has two call sites neither of which check
> the return value.  Perhaps that should be addressed in a separate
> patch, as your change here isn''t making it worse.  So no need to
do
> anything about this right now if you don''t want to.
> Yes, it isn''t part of what we intended to change.  There are other
things too
with respect to disk configuration option code that requires revisiting.
>> -        case PHYSTYPE_QCOW2:
>> -        case PHYSTYPE_VHD:
>> +        case DISK_BACKEND_TAP:
>> +        case DISK_BACKEND_QDISK: {
> 
> Once again, this is a stylistic change.  These { } are not used
> elsewhere in libxl so you should not introduce them.  (It would be a
> different matter if there were some reason why they are required in a
> particular case, eg to introduce a relevant block scope.)
> I agree, will change.
>>          case DSTATE_PHYSPATH:
>> -            if ( *p == '','' ) {
>> +            if (*p == '','') {
>>                  int ioemu_len;
> 
> It is good that you fixed up the formatting in code you changed, but
> please do not make formatting changes to unchanged lines.
>Argg...  I was worried you might ask why I didn''t fix the style around
the code
I changed.  I have reverted the changes.
> We will do a style cleanup after 4.1 is released.
> 
>> -                if ( tok + ioemu_len < end &&
>> +                if (tok + ioemu_len < end &&
> 
> Once again.  Can you make sure your patch doesn''t have /any/
unrelated
> formatting changes to lines you don''t touch, please ?
>Yes.

I will send a patch momentarily.

Kamala

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-14 19:51 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Attached patch should address your concerns.

Posting the patch description again for convenience -

Per suggestion, along with the latest patch is a more detailed description to
add to the check-in message -

Currently we pile all the backend and format information pertaining to disk
option in a single enum.  This check-in separates the two and uses two enums,
one for disk format and another for disk backend.  This helps clearly
differentiate between disk format and backend within the implementation and also
helps cleanup the code in this area in preparation for the impending parser
revamping to be done post 4.1.  Along with separating format and backend, this
check-in also removes unwanted types and renames variables in the disk interface
and fixes the code affected by the interface changes.

In specific, here are the disk interface changes made - In libxl_device_disk
structure physpath was renamed to pdev_path, virtpath was renamed to vdev,
phystype was removed and replaced with backend and format enums.  Also
previously a single enum libxl_disk_phystype held the values for qcow, qcow2,
vhd, aio, file, phy, empty and that got refactored into two enums,
libxl_disk_format to hold unknown, qcow, qcow2, vhd, raw, empty and
libxl_disk_backend to hold unknown, phy, tap and qdisk.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2011-Feb-15 19:22 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface change
plus changes to code it impacts"):> Attached patch should address your concerns.
Thanks for this.  However, there were other changes made between
this most recent version and the previous version, besides the ones
I mentioned in my comments and which you said you''d address.

When you post an updated version of a patch you should state
separately
 - what the patch does to the tree, including intended changes,
   motivation, etc., as you have done (for the commit message)
 - how the patch differs from the previous version, if applicable

Can you explain what the changes you made and why you made them ?
Ideally in general you should use revision control system tools to
make sure that you know what they all are.

But, for your info here is the output of
  diff -w --exclude=\*{~,.o,.d,.opic} -ru A B |grep -v ''^Only
in''

Most of this is formatting noise, which addresses my comments.

However, you have also:
  * Initialised disk->format and disk->backend somewhere you
    previously didn''t
  * Recognised the "tap2" prefix in a place you previously
didn''t
  * Changed the handling of the "aio" and "raw" prefixes

Normally patch such as this one, which is presented as being mature,
ought not to need unexplained semantic changes at this late stage.

Ian.

diff -w --exclude=''*~'' --exclude=''*.o''
--exclude=''*.d'' --exclude=''*.opic'' -ru
tools/libxl/libxl.c /u/iwj/work/xen-unstable-tools.hg/tools/libxl/libxl.c
--- tools/libxl/libxl.c	2011-02-15 19:13:12.000000000 +0000
+++ /u/iwj/work/xen-unstable-tools.hg/tools/libxl/libxl.c	2011-02-15
19:12:48.000000000 +0000
@@ -931,7 +931,7 @@
     device.kind = DEVICE_VBD;
 
     switch (disk->backend) {
-        case DISK_BACKEND_PHY: {
+        case DISK_BACKEND_PHY: 
             libxl__device_physdisk_major_minor(disk->pdev_path, &major,
&minor);
             flexarray_append(back, "physical-device");
             flexarray_append(back, libxl__sprintf(&gc, "%x:%x",
major, minor));
@@ -941,9 +941,8 @@
 
             device.backend_kind = DEVICE_VBD;
             break;
-        }
         case DISK_BACKEND_TAP:
-        case DISK_BACKEND_QDISK: {
+        case DISK_BACKEND_QDISK: 
             if (disk->format == DISK_FORMAT_EMPTY)
                 break;
             if (libxl__blktap_enabled(&gc)) {
@@ -972,12 +971,11 @@
                           libxl__device_disk_string_of_format(disk->format),
disk->pdev_path));
 
             if (libxl__blktap_enabled(&gc) && 
-                 disk->format != DISK_BACKEND_QDISK)
+                 disk->backend != DISK_BACKEND_QDISK)
                 device.backend_kind = DEVICE_TAP;
             else
                 device.backend_kind = DEVICE_QDISK;
             break;
-        }
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend
type: %d\n", disk->backend);
             rc = ERROR_INVAL;
@@ -1053,7 +1051,7 @@
     char *ret = NULL;
 
     switch (disk->backend) {
-        case DISK_BACKEND_PHY: {
+        case DISK_BACKEND_PHY: 
             if (disk->format != DISK_FORMAT_RAW) {
                 LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "physical block device
must"
                     " be raw");
@@ -1063,8 +1061,7 @@
                 disk->pdev_path);
             dev = disk->pdev_path;
             break;
-        }
-        case DISK_BACKEND_TAP: {
+        case DISK_BACKEND_TAP: 
             if (disk->format == DISK_FORMAT_VHD || disk->format ==
DISK_FORMAT_RAW)
             {
                 if (libxl__blktap_enabled(&gc))
@@ -1091,8 +1088,7 @@
                     "type: %d", disk->backend);
                 break;
             }
-        }
-        case DISK_BACKEND_QDISK: {
+        case DISK_BACKEND_QDISK: 
             if (disk->format != DISK_FORMAT_RAW) {
                 LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot locally attach a
qdisk "
                     "image if the format is not raw");
@@ -1102,15 +1098,12 @@
                 disk->pdev_path);
             dev = disk->pdev_path;
             break;
-
-        }
         case DISK_BACKEND_UNKNOWN:
-        default: {
+        default: 
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend
"
                 "type: %d", disk->backend);
             break;
         }
-    }
 
     if (dev != NULL)
         ret = strdup(dev);
diff -w --exclude=''*~'' --exclude=''*.o''
--exclude=''*.d'' --exclude=''*.opic'' -ru
tools/libxl/xl_cmdimpl.c
/u/iwj/work/xen-unstable-tools.hg/tools/libxl/xl_cmdimpl.c
--- tools/libxl/xl_cmdimpl.c	2011-02-15 19:13:12.000000000 +0000
+++ /u/iwj/work/xen-unstable-tools.hg/tools/libxl/xl_cmdimpl.c	2011-02-15
19:12:48.000000000 +0000
@@ -452,6 +452,8 @@
     char *p, *end, *tok;
 
     memset(disk, 0, sizeof(*disk));
+    disk->format = DISK_FORMAT_RAW;
+    disk->backend = DISK_BACKEND_TAP;
 
     for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) {
         switch(state){
@@ -466,7 +468,8 @@
                     state = DSTATE_PHYSPATH;
                     disk->format = DISK_FORMAT_RAW;
                     disk->backend = DISK_BACKEND_TAP;
-                }else if (!strcmp(tok, "tap")) {
+                }else if ((!strcmp(tok, "tap")) ||
+                          (!strcmp(tok, "tap2"))) {
                     state = DSTATE_TAP;
                 }else{
                     fprintf(stderr, "Unknown disk type: %s\n", tok);
@@ -485,9 +488,10 @@
             if ( *p == '':'' ) {
                 *p = ''\0'';
                 if (!strcmp(tok, "aio")) {
-                    disk->format = DISK_FORMAT_RAW;
-                    disk->backend = DISK_BACKEND_TAP;
-                }else if (!strcmp(tok, "vhd")) {
+                    tok = p + 1;
+                    break;
+                }
+                if (!strcmp(tok, "vhd")) {
                     disk->format = DISK_FORMAT_VHD;
                     disk->backend = DISK_BACKEND_TAP;
                 }else if (!strcmp(tok, "qcow")) {
@@ -496,7 +500,11 @@
                 }else if (!strcmp(tok, "qcow2")) {
                     disk->format = DISK_FORMAT_QCOW2;
                     disk->backend = DISK_BACKEND_QDISK;
-                }else {
+                }else if (!strcmp(tok, "raw")) {
+                    disk->format = DISK_FORMAT_RAW;
+                    disk->backend = DISK_BACKEND_TAP;
+                }
+                else {
                     fprintf(stderr, "Unknown tapdisk type: %s\n",
tok);
                     return 0;
                 }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kamala Narasimhan

2011-Feb-15 19:38 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Ian Jackson wrote:> Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface
change plus changes to code it impacts"):
>> Attached patch should address your concerns.
> 
> Thanks for this.  However, there were other changes made between
> this most recent version and the previous version, besides the ones
> I mentioned in my comments and which you said you''d address.
> 
> When you post an updated version of a patch you should state
> separately
>  - what the patch does to the tree, including intended changes,
>    motivation, etc., as you have done (for the commit message)
>  - how the patch differs from the previous version, if applicable
> 
> Can you explain what the changes you made and why you made them ?
> Ideally in general you should use revision control system tools to
> make sure that you know what they all are.
> 
> But, for your info here is the output of
>   diff -w --exclude=\*{~,.o,.d,.opic} -ru A B |grep -v ''^Only
in''
> 
> Most of this is formatting noise, which addresses my comments.
> 
> However, you have also:
>   * Initialised disk->format and disk->backend somewhere you
>     previously didn''t
>   * Recognised the "tap2" prefix in a place you previously
didn''t
>   * Changed the handling of the "aio" and "raw"
prefixes
>Yes, they are all changes made based on input from Stefano and in an attempt to
be in sync with the documentation.  Ideally we wouldn''t need these
changes if we
were to go with the rest of the patches but since we made a decision not to go
with the rest before 4.2, we decided to do some minimal changes to sync with the
doc.
> Normally patch such as this one, which is presented as being mature,
> ought not to need unexplained semantic changes at this late stage.I think we are beating to death an already dull set of changes which makes it
all the more uninteresting to me :)  But I digress...

Just to be clear - Would you accept the patch I sent earlier if I extended the
description to include the fact that I initialize disk format, backend to
default values and changed how we handle "aio" prefix plus recognize
"raw" and
"tap2" prefixes now?

Kamala



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2011-Feb-15 19:41 UTC

head link

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Kamala Narasimhan writes ("Re: [xen-devel][PATCH 2/5] Xl interface change
plus changes to code it impacts"):> Yes, they are all changes made based on input from Stefano and in an
> attempt to be in sync with the documentation.  Ideally we wouldn''t
> need these changes if we were to go with the rest of the patches but
> since we made a decision not to go with the rest before 4.2, we
> decided to do some minimal changes to sync with the doc.
Right, that makes sense.
> Just to be clear - Would you accept the patch I sent earlier if I
> extended the description to include the fact that I initialize disk
> format, backend to default values and changed how we handle "aio"
> prefix plus recognize "raw" and "tap2" prefixes now?
Thanks, I just wanted an explanation.  I will apply the series now.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Feb 2011 - [PATCH 0/5] xl disk configuration handling

[xen-devel][PATCH 0/5] xl disk configuration handling

[xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 0/5] xl disk configuration handling

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts

Re: [xen-devel][PATCH 2/5] Xl interface change plus changes to code it impacts